Re: [dpdk-dev] [PATCH v4 7/7] ethdev: hide eth dev related structures

[David Marchand <david.marchand@redhat.com>]

On Tue, Oct 5, 2021 at 12:43 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > This change is going to hurt a lot of people :-).
> > But this is a necessary move.
> >
>
> +1 that it is necessary move, but I am surprised to see how much 'rte_eth_devices'
> is accessed directly.
>
> Do you have any idea/suggestion on how can we reduce the pain for them?

From what I see, ethdev iterators are probably something that is not
known enough.
rte_eth_dev_info_get() also fills some other spots.
I don't have a magic answer, people need to look at existing API.


But I just scratched the surface, looking at rte_eth_devices[] accesses.
There might be other rte_eth_dev object dereferences (you get one
point calling rte_eth_dev_info_get) that my grep did not catch.


Details:

>
> > $ git grep-all -lw rte_eth_devices |grep -v \\.patch$
> > ANS/ans/ans_main.c

I think this code is just lagging behind what ethdev currently
provides/does wrt default offload config.
This code probably does not need to dereference rte_eth_devices[] to
query offloads configuration.


> > BESS/core/drivers/pmd.cc

ethdev iterators can replace those accesses.


> > dma_ip_drivers/QDMA/DPDK/drivers/net/qdma/qdma_xdebug.c
> > dma_ip_drivers/QDMA/DPDK/drivers/net/qdma/rte_pmd_qdma.c
> > dma_ip_drivers/QDMA/DPDK/examples/qdma_testapp/pcierw.c
> > dma_ip_drivers/QDMA/DPDK/examples/qdma_testapp/testapp.c

This is a DPDK clone, with an additional driver, so irrelevant.


> > FD.io-VPP/src/plugins/dpdk/device/format.c

rte_eth_rx_burst_mode_get() and rte_eth_tx_burst_mode_get() should do the job.
I wonder if those APIs were introduced in DPDK for VPP.. ?


> > lagopus/src/dataplane/dpdk/dpdk_io.c

Idem, ethdev iterators and rte_eth_dev_info_get() instead of direct
access for dev_flags.


> > OVS/lib/netdev-dpdk.c

For OVS, it was ethdev iterators + rte_eth_dev_info_get() where necessary.


> > packet-journey/app/kni.c

There might be something missing in current ethdev API.
This app wants to know if device is started... but on the other hand,
that's probably something the app tracks itself.


> > pktgen-dpdk/app/pktgen-port-cfg.c
> > pktgen-dpdk/app/pktgen-port-cfg.h
> > pktgen-dpdk/app/pktgen-stats.c

Accesses to offload configuration which I think are unneeded (like ANS).
Direct access for dev_flags, can be replaced with rte_eth_dev_info_get.
Direct access for name, can be replaced with rte_eth_dev_info_get.


> > Trex/src/dpdk_funcs.c
> > Trex/src/drivers/trex_i40e_fdir.c
> > Trex/src/drivers/trex_ixgbe_fdir.c

Here, there is some horror.
Directly casting and accessing hardware:
    struct rte_eth_dev *dev = &rte_eth_devices[repid];
        struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);

        I40E_WRITE_REG(hw, I40E_GLQF_ORT(12), 0x00000062);
        I40E_WRITE_REG(hw, I40E_GLQF_PIT(2), 0x000024A0);
        I40E_WRITE_REG(hw,
I40E_PRTQF_FD_INSET(I40E_FILTER_PCTYPE_NONF_IPV4_UDP, 0), 0);
etc...

This code probably bypasses too much of dpdk API, I stopped at this.


> > TungstenFabric-vRouter/gdb/vr_dpdk.gdb

Mm, interesting, this part displays DPDK internals from gdb.
That's something I have in my todolist for a long time, providing some
common gdb scripts in DPDK...


-- 
David Marchand