Re: [RFC] Add hot plug event in rte eal interrupt and inplement it in i40e driver.

["Wu, Jingjing" <jingjing.wu@intel.com>]

> -----Original Message-----
> From: Guo, Jia
> Sent: Sunday, May 28, 2017 11:45 PM
> To: Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Richardson,
> Bruce <bruce.richardson@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> Liu, Yuanhan <yuanhan.liu@intel.com>; gaetan.rivet@6wind.com
> Cc: dev@dpdk.org; Guo, Jia <jia.guo@intel.com>
> Subject: [dpdk-dev] [RFC] Add hot plug event in rte eal interrupt and inplement it in i40e
> driver.
> 
> For HW hotplug feature, we had already got upstream that removal event adding from 6wind
> as bellow.
> 
> dependency of “add device removal event” patch set:
> http://dpdk.org/dev/patchwork/patch/23693/
> [dpdk-dev,v2,1/5] ethdev: introduce device removal event
> http://dpdk.org/dev/patchwork/patch/23694/
> [dpdk-dev,v2,2/5] net/mlx4: device removal event support
> http://dpdk.org/dev/patchwork/patch/23695/
> [dpdk-dev,v2,3/5] app/testpmd: generic event handler
> http://dpdk.org/dev/patchwork/patch/23696/
> [dpdk-dev,v2,4/5] app/testpmd: request link status interrupt
> http://dpdk.org/dev/patchwork/patch/23697/
> [dpdk-dev,v2,5/5] app/testpmd: request device removal interrupt
> 
> From the patches, we can see a new event type “RTE_ETH_DEV_INTR_RMV” has been added
> into the ethdev, and the event has been implemented in mlx4 driver, and Testpmd be use for
> testing purposes and as a practical usage example for how to use these event. The progress is
> use the mlx4 driver to register interrupt callback function to rte eal interrupt source, and
> when rte epolling detect the IBV_EVENT_DEVICE_FATAL , which is identify the device remove
> behavior, it will callback into the driver’s interrupt handle to handle it, and then callback to
> the user app, such as testpmd, to detach the pci device.
> 
> So far, except the mlx4 driver, other driver like i40, that not have the remove interrupt from
> hw, will not be able to monitoring the remove behavior, so in order to expand the hot plug
> feature for all driver cases, something must be done ot detect the remove event at the kernel
> level and offer a new line of interrupt to the userland. The idea is coming as below.
> 
> Use I40e as example, we know that the rmv interrupt is not added in hw, but we could
> monitor the uio file descriptor to catch the device remove event as bellow.
> 
> The info of uevent form FD monitoring:
> remove@/devices/pci0000:80/0000:80:02.2/0000:82:00.0/0000:83:03.0/0000:84:00.2/uio
> /uio2
> ACTION=remove
> DEVPATH=/devices/pci0000:80/0000:80:02.2/0000:82:00.0/0000:83:03.0/0000:84:00.2/ui
> o/uio2
> SUBSYSTEM=uio
> MAJOR=243
> MINOR=2
> DEVNAME=uio2
> SEQNUM=11366
> 
> Firstly, in order to monitor the uio file descriptor, we need to create socket to monitor the uio
> fd, that is defined as “hotplug_fd”, and then add the uio fd into the epoll fd list, rte eal could
> epoll all of the interrupt event from hw interrupt and also include the uevent from
> hotplug_fd.
> 
> Secondly, in order to read out the uevent that monitoring, we need to add uevent API in rte
> layer. We plan add 2 , rte_uevent_connect and  rte_get_uevent. All driver interrupt handler
> could use these API to enable the uevent monitoring, and read out the uevent type , then
> corresponding to handle these uevent, such as detach the device when get the remove type.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c                     |  15 +++
>  lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 146 ++++++++++++++++++++-
>  .../linuxapp/eal/include/exec-env/rte_interrupts.h |  32 +++++
>  3 files changed, 191 insertions(+), 2 deletions(-)
>
It will be better to split the patch to two sub patches, one is for eal change, the other is for driver enabling.

> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 4c49673..0336f82 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -66,6 +66,8 @@
>  #include "i40e_pf.h"
>  #include "i40e_regs.h"
> 
> +extern int hotplug_fd;
> +
>  #define ETH_I40E_FLOATING_VEB_ARG	"enable_floating_veb"
>  #define ETH_I40E_FLOATING_VEB_LIST_ARG	"floating_veb_list"
> 
> @@ -5808,8 +5810,21 @@ i40e_dev_interrupt_handler(void *param)
>  {
>  	struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
>  	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct rte_uevent event;
>  	uint32_t icr0;
> 
> +	/* check device  uevent */
> +	while (rte_get_uevent(hotplug_fd, &event) > 0) {
The hotplug_fd is used as uevent fd for all devices, right? but in i40e driver, how distinguish the event is to this dev?
I saw you check the src in eal_intr_process_interrupts, but you didn't assign the fd in i40e device's intr_handle.
Is that to say all driver's callback will be triggered?

> +		if (event.subsystem == 1) {
What is the 1 meaning? 
> +			if (event.action == RTE_UEVENT_ADD) {
> +				//do nothing here
Will you plan do anything later? Such as RTE_ETH_EVENT_INTR_NEW? If no, please remove it.
And {} can be omit.

> +			} else if (event.action == RTE_UEVENT_REMOVE) {
> +				_rte_eth_dev_callback_process(dev,
> +					RTE_ETH_EVENT_INTR_RMV, NULL);
> +			}
> +		}


> +
> +	/* connection closed */
> +	if (ret == 0) {
> +		return -1;
> +	}
{} can be omit. Serval in this patch. Please check.


> +/**
> + * It read out the uevent from the specific file descriptor.
> + *
> + * @param fd
> + *   The fd which the uevent  associated to
> + * @param uevent
> + *   Pointer to the uevent which read from the monitoring fd.
> + * @return
> + *   - On success, one.
> + *   - On failure, zeor or a negative value.
Zeor -> zero
Generally speaking, we are using negative value to indicate failure, and 0 indicate success. Expect the result has more than two options (success, failure).

> +int
> +rte_get_uevent(int fd, struct rte_uevent *uevent);
> +
> +/**
> + * Connect to the device uevent file descriptor.
> + * @return
> + *   return the connected uevent fd.
> + */
Any return code for failure?

Thanks
Jingjing