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

["Ananyev, Konstantin" <konstantin.ananyev@intel.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.
> 

Indeed, that's a quite lot...
Thanks a lot for such detailed list, very interesting.
Wonder should we heads up coming changes to these guys...
Or might be interested persons are already aware (by reading dev@dpdk.org or so).

> 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