[dpdk-dev] [PATCH v3 19/22] net/ena: make ethdev references smp safe

[Michał Krawczyk]

pt., 7 maj 2021 o 18:49 Ferruh Yigit <ferruh.yigit at intel.com> napisał(a):
>
> On 5/6/2021 3:25 PM, Michal Krawczyk wrote:
> > From: Stanislaw Kardach <kda at 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 at semihalf.com>
> > Reviewed-by: Michal Krawczyk <mk at semihalf.com>
> > Reviewed-by: Igor Chauskin <igorch at amazon.com>
> > Reviewed-by: Shay Agroskin <shayagr at 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.
>