[v3,19/22] net/ena: make ethdev references smp safe

  From: Stanislaw Kardach <kda@semihalf.com>

rte_pci_device and rte_eth_dev are process-local structures. Therefore
ena_adapter::pdev and ena_adapter::rte_dev cannot be used universally.
Switch this to extracting those structures via rte_eth_devices indexing
and remove pdev since it's not used outside of init.

Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
Reviewed-by: Michal Krawczyk <mk@semihalf.com>
Reviewed-by: Igor Chauskin <igorch@amazon.com>
Reviewed-by: Shay Agroskin <shayagr@amazon.com>
---
 drivers/net/ena/ena_ethdev.c | 47 ++++++++++++++++++------------------
 drivers/net/ena/ena_ethdev.h |  5 ++--
 2 files changed, 25 insertions(+), 27 deletions(-)
  

On 5/6/2021 3:25 PM, Michal Krawczyk wrote:
> From: Stanislaw Kardach <kda@semihalf.com>
> 
> rte_pci_device and rte_eth_dev are process-local structures. Therefore
> ena_adapter::pdev and ena_adapter::rte_dev cannot be used universally.
> Switch this to extracting those structures via rte_eth_devices indexing
> and remove pdev since it's not used outside of init.
> 
> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> Reviewed-by: Michal Krawczyk <mk@semihalf.com>
> Reviewed-by: Igor Chauskin <igorch@amazon.com>
> Reviewed-by: Shay Agroskin <shayagr@amazon.com>

<...>

> @@ -1634,7 +1633,7 @@ static void ena_timer_wd_callback(__rte_unused struct rte_timer *timer,
>  				  void *arg)
>  {
>  	struct ena_adapter *adapter = arg;
> -	struct rte_eth_dev *dev = adapter->rte_dev;
> +	struct rte_eth_dev *dev = &rte_eth_devices[adapter->port_id];
>  

I think better to try to avoid using 'rte_eth_devices' global variable as much
as possible, although it may not be possible always, but for this case it seems
it can be done.

Why not just pass "struct rte_eth_dev" as 'arg', instead of "struct ena_adapter"?

>  	check_for_missing_keep_alive(adapter);
>  	check_for_admin_com_state(adapter);
> @@ -1819,11 +1818,10 @@ static int eth_ena_dev_init(struct rte_eth_dev *eth_dev)
>  	memset(adapter, 0, sizeof(struct ena_adapter));
>  	ena_dev = &adapter->ena_dev;
>  
> -	adapter->rte_eth_dev_data = eth_dev->data;
> -	adapter->rte_dev = eth_dev;
> +	adapter->edev_data = eth_dev->data;
> +	adapter->port_id = eth_dev->data->port_id;
>  
>  	pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
> -	adapter->pdev = pci_dev;
>  
>  	PMD_INIT_LOG(INFO, "Initializing %x:%x:%x.%d",
>  		     pci_dev->addr.domain,
> @@ -1843,7 +1841,8 @@ static int eth_ena_dev_init(struct rte_eth_dev *eth_dev)
>  	}
>  
>  	ena_dev->reg_bar = adapter->regs;
> -	ena_dev->dmadev = adapter->pdev;
> +	/* This is a dummy pointer for ena_com functions. */
> +	ena_dev->dmadev = adapter;
>  
>  	adapter->id_number = adapters_found;
>  
> @@ -1857,7 +1856,7 @@ static int eth_ena_dev_init(struct rte_eth_dev *eth_dev)
>  	}
>  
>  	/* device specific initialization routine */
> -	rc = ena_device_init(ena_dev, &get_feat_ctx, &wd_state);
> +	rc = ena_device_init(ena_dev, pci_dev, &get_feat_ctx, &wd_state);
>  	if (rc) {
>  		PMD_INIT_LOG(CRIT, "Failed to init ENA device");
>  		goto err;
> @@ -2716,7 +2715,7 @@ static int ena_xstats_get_names(struct rte_eth_dev *dev,
>  				struct rte_eth_xstat_name *xstats_names,
>  				unsigned int n)
>  {
> -	unsigned int xstats_count = ena_xstats_calc_num(dev);
> +	unsigned int xstats_count = ena_xstats_calc_num(dev->data);
>  	unsigned int stat, i, count = 0;
>  
>  	if (n < xstats_count || !xstats_names)
> @@ -2765,7 +2764,7 @@ static int ena_xstats_get(struct rte_eth_dev *dev,
>  			  unsigned int n)
>  {
>  	struct ena_adapter *adapter = dev->data->dev_private;
> -	unsigned int xstats_count = ena_xstats_calc_num(dev);
> +	unsigned int xstats_count = ena_xstats_calc_num(dev->data);
>  	unsigned int stat, i, count = 0;
>  	int stat_offset;
>  	void *stats_begin;
> @@ -2997,7 +2996,7 @@ static void ena_update_on_link_change(void *adapter_data,
>  
>  	adapter = adapter_data;
>  	aenq_link_desc = (struct ena_admin_aenq_link_change_desc *)aenq_e;
> -	eth_dev = adapter->rte_dev;
> +	eth_dev = &rte_eth_devices[adapter->port_id];
>  

Similarly, instead of passing "struct ena_adapter" as "void *adapter_data", it
can be possible to pass "struct rte_eth_dev", as far as I can that chain has
more layers but still possible.


Meanhile, you can use "void *process_private;" field of "struct rte_eth_dev" to
keep process private eth_dev data, if you want.
  

pt., 7 maj 2021 o 18:49 Ferruh Yigit <ferruh.yigit@intel.com> napisał(a):
>
> On 5/6/2021 3:25 PM, Michal Krawczyk wrote:
> > From: Stanislaw Kardach <kda@semihalf.com>
> >
> > rte_pci_device and rte_eth_dev are process-local structures. Therefore
> > ena_adapter::pdev and ena_adapter::rte_dev cannot be used universally.
> > Switch this to extracting those structures via rte_eth_devices indexing
> > and remove pdev since it's not used outside of init.
> >
> > Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> > Reviewed-by: Michal Krawczyk <mk@semihalf.com>
> > Reviewed-by: Igor Chauskin <igorch@amazon.com>
> > Reviewed-by: Shay Agroskin <shayagr@amazon.com>
>
> <...>
>
> > @@ -1634,7 +1633,7 @@ static void ena_timer_wd_callback(__rte_unused struct rte_timer *timer,
> >                                 void *arg)
> >  {
> >       struct ena_adapter *adapter = arg;
> > -     struct rte_eth_dev *dev = adapter->rte_dev;
> > +     struct rte_eth_dev *dev = &rte_eth_devices[adapter->port_id];
> >
>
> I think better to try to avoid using 'rte_eth_devices' global variable as much
> as possible, although it may not be possible always, but for this case it seems
> it can be done.
>
> Why not just pass "struct rte_eth_dev" as 'arg', instead of "struct ena_adapter"?

Good suggestion, it will be changed in the v4.

>
> >       check_for_missing_keep_alive(adapter);
> >       check_for_admin_com_state(adapter);
> > @@ -1819,11 +1818,10 @@ static int eth_ena_dev_init(struct rte_eth_dev *eth_dev)
> >       memset(adapter, 0, sizeof(struct ena_adapter));
> >       ena_dev = &adapter->ena_dev;
> >
> > -     adapter->rte_eth_dev_data = eth_dev->data;
> > -     adapter->rte_dev = eth_dev;
> > +     adapter->edev_data = eth_dev->data;
> > +     adapter->port_id = eth_dev->data->port_id;
> >
> >       pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
> > -     adapter->pdev = pci_dev;
> >
> >       PMD_INIT_LOG(INFO, "Initializing %x:%x:%x.%d",
> >                    pci_dev->addr.domain,
> > @@ -1843,7 +1841,8 @@ static int eth_ena_dev_init(struct rte_eth_dev *eth_dev)
> >       }
> >
> >       ena_dev->reg_bar = adapter->regs;
> > -     ena_dev->dmadev = adapter->pdev;
> > +     /* This is a dummy pointer for ena_com functions. */
> > +     ena_dev->dmadev = adapter;
> >
> >       adapter->id_number = adapters_found;
> >
> > @@ -1857,7 +1856,7 @@ static int eth_ena_dev_init(struct rte_eth_dev *eth_dev)
> >       }
> >
> >       /* device specific initialization routine */
> > -     rc = ena_device_init(ena_dev, &get_feat_ctx, &wd_state);
> > +     rc = ena_device_init(ena_dev, pci_dev, &get_feat_ctx, &wd_state);
> >       if (rc) {
> >               PMD_INIT_LOG(CRIT, "Failed to init ENA device");
> >               goto err;
> > @@ -2716,7 +2715,7 @@ static int ena_xstats_get_names(struct rte_eth_dev *dev,
> >                               struct rte_eth_xstat_name *xstats_names,
> >                               unsigned int n)
> >  {
> > -     unsigned int xstats_count = ena_xstats_calc_num(dev);
> > +     unsigned int xstats_count = ena_xstats_calc_num(dev->data);
> >       unsigned int stat, i, count = 0;
> >
> >       if (n < xstats_count || !xstats_names)
> > @@ -2765,7 +2764,7 @@ static int ena_xstats_get(struct rte_eth_dev *dev,
> >                         unsigned int n)
> >  {
> >       struct ena_adapter *adapter = dev->data->dev_private;
> > -     unsigned int xstats_count = ena_xstats_calc_num(dev);
> > +     unsigned int xstats_count = ena_xstats_calc_num(dev->data);
> >       unsigned int stat, i, count = 0;
> >       int stat_offset;
> >       void *stats_begin;
> > @@ -2997,7 +2996,7 @@ static void ena_update_on_link_change(void *adapter_data,
> >
> >       adapter = adapter_data;
> >       aenq_link_desc = (struct ena_admin_aenq_link_change_desc *)aenq_e;
> > -     eth_dev = adapter->rte_dev;
> > +     eth_dev = &rte_eth_devices[adapter->port_id];
> >
>
> Similarly, instead of passing "struct ena_adapter" as "void *adapter_data", it
> can be possible to pass "struct rte_eth_dev", as far as I can that chain has
> more layers but still possible.
>

Yes, it's being executed directly in the callback associated with the
admin interrupt, so as long as the interrupt callbacks are being
executed in the same process which registered them (and from what I
read in the documentation, it should be so, but please correct me if
I'm wrong), the "struct rte_eth_dev" pointer should be valid.

>
> Meanhile, you can use "void *process_private;" field of "struct rte_eth_dev" to
> keep process private eth_dev data, if you want.
>