[dpdk-dev] [PATCH v2 2/6] ethdev: add port ownership

[Matan Azrad]

Hi Neil

From: Neil Horman, Thursday, January 18, 2018 3:21 PM
> On Wed, Jan 17, 2018 at 05:58:07PM +0000, Matan Azrad wrote:
> >
> > Hi Neil
> >
> >  From: Neil Horman, Wednesday, January 17, 2018 4:00 PM
> > > On Wed, Jan 17, 2018 at 12:05:42PM +0000, Matan Azrad wrote:
<snip>
> > > Matan is correct here, there is no way to preform parallel set
> > > operations using just and atomic variable here, because multiple
> > > reads of next_owner_id need to be preformed while it is stable.
> > > That is to say rte_eth_next_owner_id must be compared to
> > > RTE_ETH_DEV_NO_OWNER and owner_id in rte_eth_is_valid_owner_id.
> If
> > > you were to only use an atomic_read on such a variable, it could be
> > > incremented by the owner_new function between the checks and an
> > > invalid owner value could become valid because  a third thread
> > > incremented the next value.  The state of next_owner_id must be kept
> > > stable during any validity checks
> > >
> > > That said, I really have to wonder why ownership ids are really
> > > needed here at all.  It seems this design could be much simpler with
> > > the addition of a per- port lock (and optional ownership record).
> > > The API could consist of three
> > > operations:
> > >
> > > ownership_set
> > > ownership_tryset
> > > ownership_release
> > > ownership_get
> > >
> > >
> > > The first call simply tries to take the per-port lock (blocking if
> > > its already
> > > locked)
> > >
> >
> > Per port lock is not good because the ownership mechanism must to be
> synchronized with the port creation\release.
> > So the port creation and port ownership should use the same lock.
> >
> In what way do you need to synchronize with port creation?

The port release zeroes the data field of the port owner, so it should be synchronized with the ownership APIs.
The port creation should be synchronized with the port release.


>  If a port has not
> yet been created, then by definition the owner must be the thread calling
> the create function.

No, the owner can be any dpdk entity. (an application - multi\single threads\proccesses, a PMD, a library).
So the port allocation(usually done from the port PMD by one thread from one process) just should to allocate a port.


>  If you are concerned about the mechanics of the port
> data structure (i.e. the fact that rte_eth_devices is statically allocated, you
> can add a lock structure to the rte_eth_dev struct and initialize it statically
> with
> RTE_SPINLOCK_INITAIZER()
> 

The lock should be in shared memory to allow secondary processes entities to take owner safely.
 
> > I didn't find precedence for blocking function in ethdev.
> >
> Then perhaps we don't need that api call.  Perhaps ownership_tryset is
> enough.
>

As I already did :)
 
> > > The second call is a non-blocking version of the first
> > >
> > > The third unlocks the port, allowing others to take ownership
> > >
> > > The fourth returns whatever ownership record you want to encode with
> > > the lock.
> > >
> > > The addition of all this id checking seems a bit overcomplicated
> >
> > You miss the identification of the owner - we want to allow info of the
> owner for printing and easy debug.
> > And it is makes sense to manage the owner uniqueness by unique ID.
> >
> I specifically pointed that out above.  There is no reason an owernship record
> couldn't be added to the rte_eth_dev structure.
> 

Sorry, don't understand why.

> > The API already discussed a lot in the previous version, Do you really want,
> now, to open it again?
> >
> What I want is the most useful and elegant ownership API available.  If you
> think what you have is that, so be it.  I only bring this up because the amount
> of debate you and Konstantin have had over lock safety causes me to
> wonder if this isn't an overly complex design.

I think the complex design is in secondary\primary processes, not in the current port ownership.
I think there is some work to do there regardless port ownership.
I think also there is some work in progress for it.

Thanks, a lot.

> 
> Neil
> 
> 
> > > Neil
> >
> >