Re: [dpdk-dev] [PATCH 13/14] net/mlx5: update event handler for multiport IB devices

[Slava Ovsiienko <viacheslavo@mellanox.com>]

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Thursday, March 21, 2019 14:15
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Subject: RE: [PATCH 13/14] net/mlx5: update event handler for multiport IB
> devices
> 
> Thursday, March 21, 2019 10:11 AM, Viacheslav Ovsiienko:
> > Subject: [PATCH 13/14] net/mlx5: update event handler for multiport IB
> > devices
> >
> > This patch modifies asynchronous event handler to support multiport
> > Infiniband devices. Handler queries the event parameters, including
> > event source port index, and invokes the handler for specific devices
> > with appropriate port_id.
> 
> This commit should be along w/ the previous one, since interrupts will not
> work after commit #12 in this series.

It was 😊
I tried first to do these parts in single commit. But the resulting diff looked
as very weird thing - it intermixed the handler and install/uninstall
routines. So, it was intentionally  splitted in two for better readability.
Nonetheless, if you insist - the is no any problem to squash these ones.
Please, let me know.

> 
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > ---
> >  drivers/net/mlx5/mlx5_ethdev.c | 101
> > +++++++++++++++++++++------------
> > --------
> >  1 file changed, 51 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> > b/drivers/net/mlx5/mlx5_ethdev.c index 8358cd2..710e6b5 100644
> > --- a/drivers/net/mlx5/mlx5_ethdev.c
> > +++ b/drivers/net/mlx5/mlx5_ethdev.c
> > @@ -1032,66 +1032,67 @@ int mlx5_fw_version_get(struct rte_eth_dev
> > *dev, char *fw_ver, size_t fw_size)  }
> >
> >  /**
> > - * Device status handler.
> > + * Handle shared asynchronous events the NIC (removal event
> > + * and link status change). Supports multiport IB device.
> >   *
> > - * @param dev
> > - *   Pointer to Ethernet device.
> > - * @param events
> > - *   Pointer to event flags holder.
> > - *
> > - * @return
> > - *   Events bitmap of callback process which can be called immediately.
> > + * @param cb_arg
> > + *   Callback argument.
> >   */
> > -static uint32_t
> > -mlx5_dev_status_handler(struct rte_eth_dev *dev)
> > +void
> > +mlx5_dev_interrupt_handler(void *cb_arg)
> >  {
> > -	struct mlx5_priv *priv = dev->data->dev_private;
> > +	struct mlx5_ibv_shared *sh = cb_arg;
> >  	struct ibv_async_event event;
> > -	uint32_t ret = 0;
> >
> > -	if (mlx5_link_update(dev, 0) == -EAGAIN) {
> > -		usleep(0);
> > -		return 0;
> > -	}
> > -	/* Read all message and acknowledge them. */
> > +	/* Read all message from the IB device and acknowledge them. */
> >  	for (;;) {
> > -		if (mlx5_glue->get_async_event(priv->sh->ctx, &event))
> > +		struct rte_eth_dev *dev;
> > +		uint32_t tmp;
> > +
> > +		if (mlx5_glue->get_async_event(sh->ctx, &event))
> >  			break;
> > +		/* Retrieve and check IB port index. */
> > +		tmp = (uint32_t)event.element.port_num;
> > +		assert(tmp && (tmp <= sh->max_port));
> > +		if (!tmp ||
> > +		    tmp > sh->max_port ||
> > +		    sh->port[tmp - 1].port_id >= RTE_MAX_ETHPORTS) {
> > +			/*
> > +			 * Invalid IB port index or no handler
> > +			 * installed for this port.
> > +			 */
> > +			mlx5_glue->ack_async_event(&event);
> > +			continue;
> > +		}
> > +		/* Retrieve ethernet device descriptor. */
> > +		tmp = sh->port[tmp - 1].port_id;
> > +		dev = &rte_eth_devices[tmp];
> 
> Is there a guarantee that the representors ethedev indexes will be
> contiguous?
No any. And no assumptions of such kind were made during development.

> I think we need a mapping between ibdev port and ethedv index.
Yes, it is exactly done. Array contains the DPDK port_ids, array index is ibv_port.

> 
> > +		tmp = 0;
> > +		assert(dev);
> >  		if ((event.event_type == IBV_EVENT_PORT_ACTIVE ||
> > -			event.event_type == IBV_EVENT_PORT_ERR) &&
> > -			(dev->data->dev_conf.intr_conf.lsc == 1))
> > -			ret |= (1 << RTE_ETH_EVENT_INTR_LSC);
> > -		else if (event.event_type == IBV_EVENT_DEVICE_FATAL &&
> > -			dev->data->dev_conf.intr_conf.rmv == 1)
> > -			ret |= (1 << RTE_ETH_EVENT_INTR_RMV);
> > -		else
> > -			DRV_LOG(DEBUG,
> > -				"port %u event type %d on not handled",
> > -				dev->data->port_id, event.event_type);
> > +		     event.event_type == IBV_EVENT_PORT_ERR) &&
> > +			dev->data->dev_conf.intr_conf.lsc) {
> > +			mlx5_glue->ack_async_event(&event);
> > +			if (mlx5_link_update(dev, 0) == -EAGAIN) {
> > +				usleep(0);
> > +				continue;
> > +			}
> > +			_rte_eth_dev_callback_process
> > +				(dev, RTE_ETH_EVENT_INTR_LSC, NULL);
> > +			continue;
> > +		}
> > +		if (event.event_type == IBV_EVENT_DEVICE_FATAL &&
> > +		    dev->data->dev_conf.intr_conf.rmv) {
> > +			mlx5_glue->ack_async_event(&event);
> > +			_rte_eth_dev_callback_process
> > +				(dev, RTE_ETH_EVENT_INTR_RMV, NULL);
> > +			continue;
> > +		}
> > +		DRV_LOG(DEBUG,
> > +			"port %u event type %d on not handled",
> > +			dev->data->port_id, event.event_type);
> >  		mlx5_glue->ack_async_event(&event);
> >  	}
> > -	return ret;
> > -}
> > -
> > -/**
> > - * Handle interrupts from the NIC.
> > - *
> > - * @param[in] intr_handle
> > - *   Interrupt handler.
> > - * @param cb_arg
> > - *   Callback argument.
> > - */
> > -void
> > -mlx5_dev_interrupt_handler(void *cb_arg) -{
> > -	struct rte_eth_dev *dev = cb_arg;
> > -	uint32_t events;
> > -
> > -	events = mlx5_dev_status_handler(dev);
> > -	if (events & (1 << RTE_ETH_EVENT_INTR_LSC))
> > -		_rte_eth_dev_callback_process(dev,
> > RTE_ETH_EVENT_INTR_LSC, NULL);
> > -	if (events & (1 << RTE_ETH_EVENT_INTR_RMV))
> > -		_rte_eth_dev_callback_process(dev,
> > RTE_ETH_EVENT_INTR_RMV, NULL);
> >  }
> >
> >  /**
> > --
> > 1.8.3.1