Re: [dpdk-dev] [RFC PATCH 6/6] ixgbe: PMD for bifurc ixgbe net device

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Nov 26, 2014 at 08:22:05AM +0000, Liang, Cunming wrote:
> Thanks Bruce's valuable comments.
> 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Tuesday, November 25, 2014 11:01 PM
> > To: Liang, Cunming
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [RFC PATCH 6/6] ixgbe: PMD for bifurc ixgbe net device
> > 
> > On Tue, Nov 25, 2014 at 02:48:51PM +0000, Liang, Cunming wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Richardson, Bruce
> > > > Sent: Tuesday, November 25, 2014 10:34 PM
> > > > To: Liang, Cunming
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [RFC PATCH 6/6] ixgbe: PMD for bifurc ixgbe net
> > device
> > > >
> > > > On Tue, Nov 25, 2014 at 10:11:22PM +0800, Cunming Liang wrote:
> > > > > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > > > > ---
> > > > >  lib/librte_pmd_ixgbe/Makefile          |  13 +-
> > > > >  lib/librte_pmd_ixgbe/ixgbe_bifurcate.c | 303
> > > > +++++++++++++++++++++++++++++++++
> > > > >  lib/librte_pmd_ixgbe/ixgbe_bifurcate.h |  57 +++++++
> > > > >  lib/librte_pmd_ixgbe/ixgbe_rxtx.c      |  40 ++++-
> > > > >  lib/librte_pmd_ixgbe/ixgbe_rxtx.h      |  10 ++
> > > > >  5 files changed, 415 insertions(+), 8 deletions(-)
> > > > >  create mode 100644 lib/librte_pmd_ixgbe/ixgbe_bifurcate.c
> > > > >  create mode 100644 lib/librte_pmd_ixgbe/ixgbe_bifurcate.h
> > > > >
> > > >
> > > > These changes are the ones that I'm not too sure about. I'd prefer if all
> > > > material for the bifurcated driver be kept within the librte_pmd_bifurc
> > directory.
> > > [Liang, Cunming] I haven't a librte_pmd_bifurc library.
> > > So far the purpose of librte_bifurc is for device scan, not used as a pmd.
> > > During driver probe, depend on device id, it asks for correct pmd from
> > 'librte_pmd_ixgbe, librte_pmd_i40e'.
> > >
> > > > Is it possible to leave ixgbe largely unmodified and simply have the new
> > > > bifurcated driver pull in the needed ixgbe (and later i40e) functions at
> > > > compile time i.e. refer from one Makefile to the sources in the other
> > > > driver's directory?
> > > [Liang, Cunming] Nice point. If we have single directory gathering all direct ring
> > access.
> > > e.g. We have aka "librte_pmd_bifurc", inside it, we'll have bifurc_ixgbe,
> > bifurc_i40e, ...
> > > Each of them still depend on other libraries like
> > librte_pmd_ixgbe/librte_pmd_i40e.
> > > We may remove the internal dependence inside one pmd driver, but between
> > libraries we add more.
> > 
> > I'm not sure about all that. Two points:
> > 
> > * Why would we need separate subdirectories within the bifurcated driver
> > directory?
> > The *only* thing that is different between an implementation of ixgbe and i40e
> > to
> > use the bifurcated driver infrastructure is the code to map between NIC
> > descriptors
> > and rte_mbufs. All the other code would be identical as far as I can work out. So
> > the
> > only two routines that differ are going to be the rx_burst and tx_burst functions.
> [Liang, Cunming] Not really. If not using the fake page, we need to provide init/start/stop case by case.
> > So why not just pull in those two specific functions (or sets of functions) from
> > their respective drivers, and keep the rest of the codebase common? 
> 
> [Liang, Cunming] I'm not sure all the rest of codebase can be common.
> For rx/tx or queue_setup, we know it can, we already do it in xxx_rxtx.c. For other ops, may not.
> Even for the part we can, if we provide such common method template, it looks like we still need to register 'ops'.
> (e.g. xxx_init_shared_code, xxx_dev_tx/rx_init, xxx_dev_rxtx_start) They're not part of eth_dev_ops.
> If we consider more like enable all other DPDK ethdev API (by using ioctl like ethtools does).
> These message wrap and translation are definitely the case to put into such common codes.
> 
> So I agree with the idea to put more common method into librte_bifurc.
> But don't think it's good to make it as a common PMD driver.
> I still prefer ixgbe_bifurc.c in librte_pmd_ixgbe as an independent driver.
> Per codebase common, rxtx common stuffs already done in xxx_rxtx.c.
> Other common method provides by librte_bifurc, be used by each specific PMD.
> 
> > simpler than having the ixgbe driver having to be aware of whether it's operating
> > in bifurcated mode or uio/vfio/nic_uio mode, to check what operations are
> > supported
> > or not.
> [Liang, Cunming] If you go through the codes. You'll find it's not ixgbe driver to aware of these modes.
> We already have ixgbe driver and ixgbevf driver, now have ixgbe_bifurc driver, that's it.
> BTW ideally, it's better for ixgbe and ixgbevf in self-contain .c files, now all are in ixgbe_ethdev.c
> Ixgbe_bifurc  has weaker NIC control than ixgbevf, both are mainly focus on rx and tx.
> Ixgbe has full HW control, ixgbevf has limited HW control, ixgbe_bifurc no HW control.
> All of them has the same capability to do rx and tx.
> On this point of view, it makes sense to have such standalone driver.
> > 
> > * It's not really an inter-library dependency - or at least not a hugely problematic
> > one to my mind. With my proposal there is no need for the ixgbe or i40e drivers
> > to
> > be compiled up for the bifurcated driver to work with them. It simply makes use
> > of the rx and tx code functions to do the mapping from descriptors to mbufs.
> > While
> > there will be a dependency on those functions, the nice thing is that those
> > functions
> > are already standardized by the ethdev API, so we don't need to worry about
> > internal changes inside the drivers changing the APIs of those functions.
> [Liang, Cunming] I think I haven't fully got your point.
> Do you propose we don't need the specific PMD bifurc, instead to provide a driver directly on top of all other PMD ?
> We expose more low level function to ethdev API as needed.
> In this way, there's a risk that we assume kernel always guarantee the illegal register access only goes into the fake pages.
> If not, such register access by normal PMD is un-expectable. 
> 

My main thinking is that the ethdev HW APIs applicable to the bifurcated driver
are going to be very, very limited. Even the we can set up RX and TX queues
and perform RX and TX on them, but everything else, as far as I can see, is
going to be controlled externally via ethtool access to the kernel. Therefore, it
seems to me that an i40e device in bifurcated mode has much more in common with
an ixgbe device in bifurcated mode that with an i40e device being directly
controlled by DPDK. For these reasons I think a single driver for all bifurcated
drivers makes more sense.

However, without having prototyped such a scheme one cannot be sure if it will
really work, so I'm curious what other people think is the best approach to
producing such a driver.

/Bruce

> > 
> > /Bruce
> > 
> > 
> > >
> > > > My thinking is that the bifurcated driver is so significantly different in
> > > > the way it works, and the limits on it's functionality e.g. no direct filter
> > > > support or queue management, that it's best kept completely separate and
> > only
> > > > "borrow" the needed descriptor read/write functions from the other drivers
> > as is
> > > > needed.
> > > >
> > > > Just my 2c. I'm curious as to what others think.
> > > >
> > > > /Bruce
> > > >