Re: [dpdk-dev] [PATCH v3 3/8] net/failsafe: support probed sub-devices getting

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hi Matan,

On Tue, Jan 16, 2018 at 05:20:27PM +0000, Matan Azrad wrote:
> Hi Gaetan
> 

<snip>

> > > > In 18.05, or 18.08 there should be an EAL function that would be
> > > > able to identify a device given a specific ID string (very close to an
> > rte_devargs).
> > > > Currently, this API does not exist.
> > > >
> > > > You can hack your way around this for the moment, IF you really,
> > > > really
> > > > want: parse your devargs, get the bus, use the bus->parse() function
> > > > to get a binary device representation, and compare bytes per bytes
> > > > the binary representation given by your devargs and by the device-
> > >name.
> > > >
> > > > But this is a hack, and a pretty ugly one at that: you have no way
> > > > of knowing the size taken by this binary representation, so you can
> > > > restrict yourself to the vdev and PCI bus for the moment and take
> > > > the larger of an rte_vdev_driver pointer and an rte_pci_addr....
> > > >
> > > >         {
> > > >             union {
> > > >                     rte_vdev_driver *drv;
> > > >                     struct rte_pci_addr pci_addr;
> > > >             } bindev1, bindev2;
> > > >             memset(&bindev1, 0, sizeof(bindev1));
> > > >             memset(&bindev2, 0, sizeof(bindev2));
> > > >             rte_eal_devargs_parse(device->name, da1);
> > > >             rte_eal_devargs_parse(your_devstr, da2);
> > > >             RTE_ASSERT(da1->bus == rte_bus_find_by_name("pci") ||
> > > >                        da1->bus == rte_bus_find_by_name("vdev"));
> > > >             RTE_ASSERT(da2->bus == rte_bus_find_by_name("pci") ||
> > > >                        da2->bus == rte_bus_find_by_name("vdev"));
> > > >             da1->bus->parse(da1->name, &bindev1);
> > > >             da1->bus->parse(da2->name, &bindev2);
> > > >             if (memcmp(&bindev1, &bindev2, sizeof(bindev1)) == 0) {
> > > >                     /* found the device */
> > > >             } else {
> > > >                     /* not found */
> > > >             }
> > > >         }
> > > >
> > > > So, really, really ugly. Anyway.
> > > >
> > > Yes, ugly :) Thanks for this update!
> > > Will keep the comparison by device->name.
> > >
> > 
> > Well as explained, above, the comparison by device->name only works with
> > whitelisted devices.
> > 
> 
> > So either implement something broken right now that you will need to
> > update in 18.05, or implement it properly in 18.05 from the get go.
> > 
> For the current needs it is enough.
> We can also say that it is the user responsibility to pass to failsafe the same names and same args as he passes for EAL(or default EAL names).
> I think I emphasized it in documentation.
> 

Okay, as you wish. Just be aware of this limitation.

I think this functionality is good and useful, but it needs to be made clean.
The proper function should be available soon, then this implementaion should
be cleaned up.

> > > > <snip>
> > > >
> > > > > > > +			/* Take control of device probed by EAL
> > options. */
> > > > > > > +			DEBUG("Taking control of a probed sub
> > device"
> > > > > > > +			      " %d named %s", i, da->name);
> > > > > >
> > > > > > In this case, the devargs of the probed device must be copied
> > > > > > within the sub- device definition and removed from the EAL using
> > > > > > the proper rte_devargs API.
> > > > > >
> > > > > > Note that there is no rte_devargs copy function. You can use
> > > > > > rte_devargs_parse instead, "parsing" again the original devargs
> > > > > > into the sub- device one. It is necessary for complying with
> > > > > > internal rte_devargs requirements (da->args being malloc-ed, at
> > > > > > the moment,
> > > > but may evolve).
> > > > > >
> > > > > > The rte_eal_devargs_parse function is not easy enough to use
> > > > > > right now, you will have to build a devargs string (using snprintf) and
> > submit it.
> > > > > > I proposed a change this release for it but it will not make it
> > > > > > for 18.02, that would have simplified your implementation.
> > > > > >
> > > > >
> > > > > Got you. You right we need to remove the created devargs in
> > > > > fail-safe
> > > > parse level.
> > > > > What do you think about checking it in the parse level and avoid
> > > > > the new
> > > > devargs creation?
> > > > > Also to do the copy in parse level(same method as we are doing in
> > > > > probe
> > > > level)?
> > > > >
> > > >
> > > > Not sure I follow here, but the new rte_devargs is part of the
> > > > sub-device (it is not a pointer, but allocated alongside the sub_device).
> > > >
> > > > So keep everything here, it is the right place to deal with these things.
> > > >
> > > But it will prevent the double parsing and also saves the method:
> > > If the device already parsed - copy its devargs and continue.
> > > If the device already probed - copy the device pointer and continue.
> > >
> > > I think this is the right dealing, no?
> > > Why to deal with parse level in probe level?  Just keep all the parse work to
> > parse level and the probe work to probe level.
> > 
> > After re-reading, I think we misunderstood each other.
> > You cannot remove the rte_devargs created during parsing: it is allocated
> > alongside the sub_device structure.
> > 
> > You must only remove the rte_devargs allocated by the EAL (using
> > rte_eal_devargs_remove()).
> > 
> 
> Sure.
> 
> > Before removing it, you must copy its content in the local sub_device
> > rte_devargs structure. I only proposed a way to do this copy that would not
> > deal with rte_devargs internals, as it is bound to evolve rather soon.
> >
> Yes.
>  
> > Otherwise, no, I do not want to complicate the parsing operations, they are
> > already too complicated and too criticals. Better to keep it all here.
> 
> I think fs_parse_device function is not complicated and it is the natural place for devargs games.
> For me this is the right place for the copy & remove devargs.
> Are you insisting to put all in fs_bus_init?

You would have to put fs_ethdev_portid_find in failsafe_args, which is
mixing layers. Sorry but yes, please keep all these changes in this
file.

Thanks,
-- 
Gaëtan Rivet
6WIND