[dpdk-dev] [PATCH v3 7/7] app/testpmd: adjust ethdev port ownership

[Ananyev, Konstantin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas at monjalon.net]
> Sent: Wednesday, January 24, 2018 8:10 AM
> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>
> Cc: Matan Azrad <matan at mellanox.com>; Gaëtan Rivet <gaetan.rivet at 6wind.com>; Wu, Jingjing <jingjing.wu at intel.com>;
> dev at dpdk.org; Neil Horman <nhorman at tuxdriver.com>; Richardson, Bruce <bruce.richardson at intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 7/7] app/testpmd: adjust ethdev port ownership
> 
> 23/01/2018 22:18, Ananyev, Konstantin:
> > >
> > > 23/01/2018 16:18, Ananyev, Konstantin:
> > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ananyev, Konstantin
> > > > > From: Thomas Monjalon [mailto:thomas at monjalon.net]
> > > > > > 23/01/2018 14:34, Ananyev, Konstantin:
> > > > > > > If that' s the use case, then I think you need to set device ownership at creation time -
> > > > > > > inside dev_allocate().
> > > > > > > Again that would avoid such racing conditions inside testpmd.
> > > > > >
> > > > > > The devices must be allocated at a low level layer.
> > > > >
> > > > > No one arguing about that.
> > > > > But we can provide owner id information to the low level.
> > >
> > > Sorry, you did not get it.
> >
> > Might be.
> >
> > > We cannot provide owner id at the low level
> > > because it is not yet decided who will be the owner
> > > before the port is allocated.
> >
> > Why is that?
> > What prevents us decide who will manage that device *before* allocating port of it?
> > IMO we do have all needed information at that stage.
> 
> We don't have the information.

At that point we do have dev name and all parameters, right?
Plus we do have blacklist/whitelist, etc.
So what else are we missing to make the decision at that point? 

> It is a new device, it is matched by a driver which allocates a port.
> I don't see where the higher level can interact here.
> And even if you manage a trick, the higher level needs to read the port
> informations to decide the ownership.

Could you specify what particular port information it needs?

> 
> > > > > > When a new device appears (hotplug), an ethdev port should be allocated
> > > > > > automatically if it passes the whitelist/blacklist policy test.
> > > > > > Then we must decide who will manage this device.
> > > > > > I suggest notifying the DPDK libs first.
> > > > > > So a DPDK lib or PMD like failsafe can have the priority to take the
> > > > > > ownership in its notification callback.
> > > > >
> > > > > Possible, but seems a bit overcomplicated.
> > > > > Why not just:
> > > > >
> > > > > Have a global variable process_default_owner_id, that would be init once at startup.
> > > > > Have an LTS variable default_owner_id.
> > > > > It will be used by rte_eth_dev_allocate() caller can set dev->owner_id at creation time,
> > > > > so port allocation and setting ownership - will be an atomic operation.
> > > > > At the exit rte_eth_dev_allocate() will always reset default_owner_id=0:
> > > > >
> > > > > rte_eth_dev_allocate(...)
> > > > > {
> > > > >    lock(owner_lock);
> > > > >    <allocate_port>
> > > > >    owner = RTE_PER_LCORE(default_owner_id);
> > > > >    if (owner == 0)
> > > > >        owner = process_default_owner_id;
> > > > >   set_owner(port, ..., owner);
> > > > >  unlock(owner_lock);
> > > > >  RTE_PER_LCORE(default_owner_id) = 0;
> > > >
> > > > Or probably better to leave default_owner_id reset to the caller.
> > > > Another thing - we can use same LTS variable in all control ops to
> > > > allow/disallow changing of port configuration based on ownership.
> > > > Konstantin
> > > >
> > > > > }
> > > > >
> > > > > So callers who don't need any special ownership - don't need to do anything.
> > > > > Special callers (like failsafe) can set default_owenr_id just before calling hotplug
> > > > > handling routine.
> > >
> > > No, hotplug will not be a routine.
> > > I am talking about real hotplug, like a device which appears in the VM.
> > > This new device must be handled by EAL and probed automatically if
> > > comply with whitelist/blacklist policy given by the application or user.
> > > Real hotplug is asynchronous.
> >
> > By 'asynchronous' here you mean it would be handled in the EAL interrupt thread
> > or something different?
> 
> Yes, we receive an hotplug event which is processed in the event thread.
> 
> > Anyway, I suppose  you do need a function inside DPDK that will do the actual work in response
> > on hotplug event, right?
> 
> Yes

Ok, btw why that function has to be always called from interrupt thread?
Why not to allow user to decide?
Some apps have their own thread dedicated for control ops (like testpmd)
For them it might be plausible to call that function from their own control thread context.
Konstantin

> 
> > That's what I refer to as 'hotplug routine' above.
> >
> > > We will just receive notifications that it appeared.
> > >
> > > Note: there is some temporary code in failsafe to manage some hotplug.
> > > This code must be removed when it will be properly handled in EAL.
> >
> > Ok, if it is just a temporary code, that would be removed soon -
> > then it definitely seems wrong to modify tespmd (or any other user app)
> > to comply with that temporary solution.
> 
> It will be modified when EAL hotplug will be implemented.
> 
> However, the ownership issue will be the same:
> we don't know the owner when allocating a port.