[dpdk-dev] [PATCH v6 2/8] ethdev: add switch identifier parameter to port

[Shahaf Shuler]

--Shahaf


> -----Original Message-----
> From: Doherty, Declan [mailto:declan.doherty at intel.com]
> Sent: Thursday, March 29, 2018 6:13 PM
> To: Shahaf Shuler <shahafs at mellanox.com>; dev at dpdk.org
> Cc: Alex Rosenbaum <alexr at mellanox.com>; Ferruh Yigit
> <ferruh.yigit at intel.com>; Thomas Monjalon <thomas at monjalon.net>; Qi
> Zhang <qi.z.zhang at intel.com>; Alejandro Lucero
> <alejandro.lucero at netronome.com>; Andrew Rybchenko
> <arybchenko at solarflare.com>; Mohammad Abdul Awal
> <mohammad.abdul.awal at intel.com>; Remy Horton
> <remy.horton at intel.com>; John McNamara <john.mcnamara at intel.com>;
> Rony Efraim <ronye at mellanox.com>; Jingjing Wu <jingjing.wu at intel.com>;
> Wenzhuo Lu <wenzhuo.lu at intel.com>; Vincent Jardin
> <vincent.jardin at 6wind.com>; Yuanhan Liu <yliu at fridaylinux.org>; Bruce
> Richardson <bruce.richardson at intel.com>; Konstantin Ananyev
> <konstantin.ananyev at intel.com>; Zhihong Wang
> <zhihong.wang at intel.com>
> Subject: Re: [dpdk-dev][PATCH v6 2/8] ethdev: add switch identifier
> parameter to port
> 
> On 29/03/2018 11:12 AM, Shahaf Shuler wrote:
> > Thursday, March 29, 2018 12:14 PM, Doherty, Declan:
> >> On 29/03/2018 7:13 AM, Shahaf Shuler wrote:
> >>> Hi Declan,
> >>>
> >>> Thanks for the series! See some comments below
> >>>
> >>> Wednesday, March 28, 2018 4:54 PM, Declan Doherty:
> >>>> Subject: [dpdk-dev][PATCH v6 2/8] ethdev: add switch identifier
> >>>> parameter to port
> >>>>
> >>>> Introduces a new port attribute to ethdev port's which denotes the
> >>>> switch domain a port belongs to. By default all port's switch
> >>>> identifiers are the their port_id. Ports which share a common
> >>>> switch domain are configured with the same switch id.
> >>>>
> >>>> Signed-off-by: Declan Doherty <declan.doherty at intel.com>
> >>>> ---
> >>>>    app/test-pmd/config.c              | 1 +
> >>>>    lib/librte_ether/rte_ethdev.c      | 3 +++
> >>>>    lib/librte_ether/rte_ethdev.h      | 1 +
> >>>>    lib/librte_ether/rte_ethdev_core.h | 1 +
> >>>>    4 files changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> >>>> 4bb255c62..e12f8c515 100644
> >>>> --- a/app/test-pmd/config.c
> >>>> +++ b/app/test-pmd/config.c
> >>>> @@ -517,6 +517,7 @@ port_infos_display(portid_t port_id)
> >>>>    	printf("Min possible number of TXDs per queue: %hu\n",
> >>>>    		dev_info.tx_desc_lim.nb_min);
> >>>>    	printf("TXDs number alignment: %hu\n",
> >>>> dev_info.tx_desc_lim.nb_align);
> >>>> +	printf("Switch Id: %u\n", dev_info.switch_id);
> >>>>    }
> >>>>
> >>>>    void
> >>>> diff --git a/lib/librte_ether/rte_ethdev.c
> >>>> b/lib/librte_ether/rte_ethdev.c index 23857c91f..f32d18cad 100644
> >>>> --- a/lib/librte_ether/rte_ethdev.c
> >>>> +++ b/lib/librte_ether/rte_ethdev.c
> >>>> @@ -290,6 +290,8 @@ rte_eth_dev_allocate(const char *name)
> >>>>    	eth_dev = eth_dev_get(port_id);
> >>>>    	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name),
> >> "%s",
> >>>> name);
> >>>>    	eth_dev->data->port_id = port_id;
> >>>> +	eth_dev->data->switch_id = port_id;
> >>>> +	/**< Default switch_id is the port_id of the device */
> >>>
> >>> Why such default is needed? Why not let the PMD to set it always?
> >>
> >> I saw this a simple way to have a consistent default value (the
> >> port_id) for all PMDs, without the need to modify existing PMD which
> >> don't currently have any concept of a switch domain.
> >
> > The default value don't makes much sense though. By default it would
> > mean the application each port is on different switch domain. This is
> > obviously not true in case of multiple VFs
> 
> But it is the case today for all ports, that they are assumed to be in separate
> switch domains or at least in unknowable switch domains by applications,
> with the exception of LAG groups, were it is implicit that they are in the same
> domain. Even if two ports were attached to the same switch, or if you where
> using multiple VFs from the same switch there is currently no way of telling
> the application that this is the case. 

Right, but I thought your new parameter is the way :

Uint16_t switch_domain -
"Introduces a new port attribute to ethdev port's which denotes the
switch domain a port belongs to"


What would be the use case for having
> multiple VFs from the same port/switch domain in a single data path
> application. It doesn't seem likely to me that you would use hardware
> switching to move traffic between 2 VFs in the same data path application.

It is not likely. However the parameter you defined is not true only for the representor case, but for the generic case (unless you state otherwise in the documentation of it).

Maybe it can also use application like VFd to understand the connectivity between the different VFs.

My point is - unless this parameter is valid only for the reprsentor case, the default value should be the correct one. 
I think it is better to not limit this field to representor only. Extra info for application is never a bad thing. 

> 
> If you are using port representors, I assumed that you would set a valid
> switch domain id, possibly picking up the port id of the first port created on
> that switch domain, but it would be completely up to the PMD how this was
> done, I have just shown the simplest model in which the port_id of the PF is
> used for all subsequent representor ports in that switch domain, but if you
> wanted to create multiple domains within the same device, you could just
> use subsequent port representors port_ids to setup further switch domains.
> A switch_id is only meaningful within the contex of DPDK process it is running
> in.
> 
> >
> > Maybe we can define ETH_SWITCH_ID_INVALID (0) to emphasis 0 is not
> the switch_id for the PMDs which didn't implemented it.
> >
> >>
> >> Also taking the approach of just leaving it up to the PMD to decide
> >> the value would mean that some form of synchronisation would be
> >> required so that two device don't select the switch domain identifier.
> >
> > What kind of knowledge ethdev layer has to set the switche_id? It should
> be based on the underlying capabilities of the device.
> > Some devices will use the kernel sysfs  to check that.
> >
> > Maybe there are devices which are able to expose the same switch for
> multiple devices, passing the packet between them using  peer2peer trough
> the PCI.
> >
> > The point is we need to have APIs which will enable all the future flexibility.
> >
> > Maybe we can have array of switch_id which are allready taken by the
> underlying devices, and PMD will register their switch_id to it atomically. To
> ease the synchronization.
> >
> >>
> >>>
> >>>>    	eth_dev->data->mtu = ETHER_MTU;
> >>>>
> >>>>    unlock:
> >>>> @@ -2395,6 +2397,7 @@ rte_eth_dev_info_get(uint16_t port_id,
> struct
> >>>> rte_eth_dev_info *dev_info)
> >>>>    	dev_info->driver_name = dev->device->driver->name;
> >>>>    	dev_info->nb_rx_queues = dev->data->nb_rx_queues;
> >>>>    	dev_info->nb_tx_queues = dev->data->nb_tx_queues;
> >>>> +	dev_info->switch_id = dev->data->switch_id;
> >>>
> >>> Why there is a need to keep the switch_id on device data?
> >>> I think PMD to store it on its private structure and report it in
> >>> dev_info is
> >> enough.
> >>>
> >>
> >> That way would require every PMD to be modified to maintain a
> >> switch_id structure, which I know isn't a big deal, as only the
> >> device which need to support it would need it, but we would need to
> >> change the dev_info-
> >>> switch_id from being a uint16_t to being a signed value so we could
> >>> have a
> >> default of -1 for device which don't support switch domain.
> >
> > We can say also 0 is the invalid switch id.
> >
> >> I thought the former approach was cleaner.
> >>
> >>>>    }
> >>>>
> >>>>    int
> >>>> diff --git a/lib/librte_ether/rte_ethdev.h
> >>>> b/lib/librte_ether/rte_ethdev.h index 036153306..dced4fc41 100644
> >>>> --- a/lib/librte_ether/rte_ethdev.h
> >>>> +++ b/lib/librte_ether/rte_ethdev.h
> >>>> @@ -1029,6 +1029,7 @@ struct rte_eth_dev_info {
> >>>>    	/** Configured number of rx/tx queues */
> >>>>    	uint16_t nb_rx_queues; /**< Number of RX queues. */
> >>>>    	uint16_t nb_tx_queues; /**< Number of TX queues. */
> >>>> +	uint16_t switch_id; /**< Switch Domain Id */
> >>>>    };
> >>>>
> >>>>    /**
> >>>> diff --git a/lib/librte_ether/rte_ethdev_core.h
> >>>> b/lib/librte_ether/rte_ethdev_core.h
> >>>> index e5681e466..caed7a4e6 100644
> >>>> --- a/lib/librte_ether/rte_ethdev_core.h
> >>>> +++ b/lib/librte_ether/rte_ethdev_core.h
> >>>> @@ -585,6 +585,7 @@ struct rte_eth_dev_data {
> >>>>    	struct ether_addr* hash_mac_addrs;
> >>>>    	/** Device Ethernet MAC addresses of hash filtering. */
> >>>>    	uint16_t port_id;           /**< Device [external] port identifier. */
> >>>> +	uint16_t switch_id;	    /**< Switch which port is associated with
> >>>> */
> >>>>    	__extension__
> >>>>    	uint8_t promiscuous   : 1, /**< RX promiscuous mode ON(1) / OFF(0).
> >>>> */
> >>>>    		scattered_rx : 1,  /**< RX of scattered packets is ON(1) /
> >>>> OFF(0) */
> >>>> --
> >>>> 2.14.3
> >>>
> >