Re: [dpdk-dev] [PATCH v2 1/3] net/ixgbe: enable hotplug detect in ixgbe

[Jeff Guo <jia.guo@intel.com>]

On 7/9/2018 6:01 PM, Matan Azrad wrote:
> Hi
>
> From: Jeff Guo
>> On 7/9/2018 5:04 PM, Matan Azrad wrote:
>>> Hi
>>>
>>> From: Jeff Guo
>>>> hi, wenzhuo and matan.
>>>>
>>>>
>>>> On 7/9/2018 3:51 PM, Matan Azrad wrote:
>>>>> Hi
>>>>>
>>>>> From: Lu, Wenzhuo
>>>>>> Hi Jeff,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jeff Guo
>>>>>>> Sent: Monday, July 9, 2018 2:57 PM
>>>>>>> To: stephen@networkplumber.org; Richardson, Bruce
>>>>>>> <bruce.richardson@intel.com>; Yigit, Ferruh
>>>>>>> <ferruh.yigit@intel.com>; Ananyev, Konstantin
>>>>>>> <konstantin.ananyev@intel.com>; gaetan.rivet@6wind.com; Wu,
>>>> Jingjing
>>>>>>> <jingjing.wu@intel.com>; thomas@monjalon.net;
>>>> motih@mellanox.com;
>>>>>>> matan@mellanox.com;
>>>>>> Van
>>>>>>> Haaren, Harry <harry.van.haaren@intel.com>; Zhang, Qi Z
>>>>>>> <qi.z.zhang@intel.com>; He, Shaopeng <shaopeng.he@intel.com>;
>>>>>>> Iremonger, Bernard <bernard.iremonger@intel.com>;
>>>>>>> arybchenko@solarflare.com
>>>>>>> Cc: jblunck@infradead.org; shreyansh.jain@nxp.com; dev@dpdk.org;
>>>>>>> Guo, Jia <jia.guo@intel.com>; Zhang, Helin <helin.zhang@intel.com>
>>>>>>> Subject: [dpdk-dev] [PATCH v2 1/3] net/ixgbe: enable hotplug
>>>>>>> detect in ixgbe
>>>>>>>
>>>>>>> This patch aim to enable hotplug detect in ixgbe pmd driver.
>>>>>>> Firstly it set the flags RTE_PCI_DRV_INTR_RMV in drv_flags to
>>>>>>> announce the hotplug ability, and then use
>>>>>>> rte_dev_event_callback_register to register the hotplug event
>>>>>>> callback to eal. When eal detect the hotplug event, it will call
>>>>>>> the callback to process it, if the event is hotplug remove, it
>>>>>>> will trigger the RTE_ETH_EVENT_INTR_RMV event into ethdev
>> callback to let app process the hotplug for the ethdev.
>>>>>>> This is an example for other driver, that if any driver support
>>>>>>> hotplug feature could be use this way to enable hotplug detect.
>>>>>>>
>>>>>>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>>>>>>> ---
>>>>>>> v2->v1:
>>>>>>> refine some doc.
>>>>>>> ---
>>>>>>>     drivers/net/ixgbe/ixgbe_ethdev.c | 46
>>>>>>> +++++++++++++++++++++++++++++++++++++++-
>>>>>>>     1 file changed, 45 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> index 87d2ad0..83ce026 100644
>>>>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>>>>> @@ -1534,6 +1534,47 @@ generate_random_mac_addr(struct
>>>> ether_addr
>>>>>>> *mac_addr)
>>>>>>>     	memcpy(&mac_addr->addr_bytes[3], &random, 3);  }
>>>>>>>
>>>>>>> +static void
>>>>>>> +eth_dev_event_callback(char *device_name, enum
>>>> rte_dev_event_type
>>>>>>> type,
>>>>>>> +		       __rte_unused void *arg)
>>>>>>> +{
>>>>>>> +	uint32_t pid;
>>>>>>> +
>>>>>>> +	if (type >= RTE_DEV_EVENT_MAX) {
>>>>>>> +		fprintf(stderr, "%s called upon invalid event %d\n",
>>>>>>> +			__func__, type);
>>>>>>> +		fflush(stderr);
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	switch (type) {
>>>>>>> +	case RTE_DEV_EVENT_REMOVE:
>>>>>>> +		PMD_DRV_LOG(INFO, "The device: %s has been
>>>>>> removed!\n",
>>>>>>> +			    device_name);
>>>>>>> +
>>>>>>> +		if (!device_name)
>>>>>>> +			return;
>>>>>>> +
>>>>>>> +		for (pid = 0; pid < RTE_MAX_ETHPORTS; pid++) {
>>>>>>> +			if (rte_eth_devices[pid].device) {
>>>>>>> +				if (!strcmp(device_name,
>>>>>>> +				    rte_eth_devices[pid].device-
>>> name)) {
>>>>>>> +
>> 	_rte_eth_dev_callback_process(
>>>>>>> +
>> 	&rte_eth_devices[pid],
>>>>>>> +
>> 	RTE_ETH_EVENT_INTR_RMV,
>>>>>>> NULL);
>>>>>>> +					continue;
>>>>>>> +				}
>>>>>>> +			}
>>>>>>> +		}
>>>>>>> +		break;
>>>>>>> +	case RTE_DEV_EVENT_ADD:
>>>>>>> +		RTE_LOG(INFO, EAL, "The device: %s has been
>> added!\n",
>>>>>>> +			device_name);
>>>>>>> +		break;
>>>>>>> +	default:
>>>>>>> +		break;
>>>>>>> +	}
>>>>>>> +}
>>>>>> I don't get the point. Looks like this's a very common rte code.
>>>>>> Why is it put in ixgbe pmd?
>>>>> Jeff needs to detect if the removed device is related to this PMD,
>>>>> than to
>>>> raise RMV events for all this PMD ethdev associated ports.
>>>>> He should not raise RMV events for other PMD  ports.
>>>>>
>>>> It should be like wenzhuo said that i could no strong reason to let
>>>> common way in ixgbe pmd.  And sure raise RMV events for none related
>>>> PMD ports is not my hope.
>>>> Will plan to let it go into the eth dev layer to process it.
>>>>
>>> How can you run ethdev function from EAL context?
>>> How can the ethdev layer know which ports are related to the EAL device
>> removal?
>>> How can ethdev layer know if the port supports removal?
>> i mean that still let driver manage the callback , just let the common ethdev
>> functional in ethdev layer.
>> It just define "rte_eth_dev_event_callback" in ethdev layer, and register the
>> common ethdev callback in pmd driver as bellow. the eth_dev could be pass
>> by the whole process.
>>
>>       rte_dev_event_callback_register(eth_dev->device->name,
>>                       rte_eth_dev_event_callback,
>>                       (void *)eth_dev);
>>
> Sorry, but I don't understand, can you explain step by step the notification path?

the step should be:
1) add a ethdev driver api "rte_dev_event_callback_register" in the 
rte_ethdev_driver.h, let pmd driver call it.
      rte_eth_dev_event_callback(char *device_name, enum 
rte_dev_event_type event, void *cb_arg);

2) register eth eal device event callback in pmd driver as below, the 
rte eth (eth_dev) could be set to cb_arg of the callback.

rte_dev_event_callback_register(eth_dev->device->name,
                      rte_eth_dev_event_callback,
                      (void *)eth_dev);

3)when hotplug event detect, the callback be called, then could be use 
the device name of the eth_dev to compare the hotplug eal device name.

4) go to the common way of RMV eth dev hotplug process.