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

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Jan 19, 2018 at 10:44:32AM +0000, Matan Azrad wrote:
> Hi Bruce
> From: Bruce Richardson, Friday, January 19, 2018 11:30 AM
> > On Fri, Jan 19, 2018 at 07:14:17AM +0000, Matan Azrad wrote:
> > >
> > > Hi Neil
> > > From: Neil Horman, Friday, January 19, 2018 3:41 AM
> > > > On Thu, Jan 18, 2018 at 08:21:34PM +0000, Matan Azrad wrote:
> > > > > Hi Neil.
> > > > >
> > > > > From: Neil Horman, Thursday, January 18, 2018 8:42 PM
> > >
> > > <snip>
> > > > > 1. What exactly do you want to improve?(in details) 2. Which API
> > > > > specifically do you want to change(\ part of code)?
> > > > > 3. What is the missing in current code(you can answer it in V3 I
> > > > > sent if you
> > > > want) which should be fixed?
> > > > >
> > > > >
> > > > > <snip> sorry for that, I think it is not relevant continue
> > > > > discussion if we are
> > > > not fully understand each other. So let's start from the beginning
> > > > "with good order :)" by answering the above questions.
> > > >
> > > >
> > > > Sure, this seems like a reasonable way to level set.
> > > >
> > > > I mentioned in another thread that perhaps some of my issue here is
> > > > perception regarding what is meant by ownership.  When I think of an
> > > > ownership api I think primarily of mutual exclusion (that is to say,
> > > > enforcement of a single execution context having access to a
> > > > resource at any given time.  In my mind the simplest form of
> > > > ownership is a spinlock or a mutex.  A single execution context
> > > > either does or does not hold the resource at any one time.  Those
> > > > contexts that attempt to gain excusive access to the resource call
> > > > an api that (depending on
> > > > implementation) either block continued execution of that thread
> > > > until exclusive access to the resource can be granted, or returns
> > > > immediately with a success or error indicator to let the caller know if
> > access is granted.
> > > >
> > > > If I were to codify this port ownership api in pseudo code it would
> > > > look something like this:
> > > >
> > > > struct rte_eth_dev {
> > > >
> > > > 	< eth dev bits >
> > > > 	rte_spinlock_t owner_lock;
> > > > 	bool locked;
> > > > 	pid_t owner_pid;
> > > > }
> > > >
> > As an aside, if you ensure that both locked (or "owned", I think in this
> > context) and owner_pid are integer values, you can do away with the lock
> > and use a compare-and-set to take ownership, by setting both atomically if
> > unmodified from the originally read values.
> > 
> > > >
> > > > bool rte_port_claim_ownership(struct rte_eth_dev *dev) {
> > > > 	bool ret = false;
> > > >
> > > > 	spin_lock(dev->owner_lock);
> > > > 	if (dev->locked)
> > > > 		goto out;
> > > > 	dev->locked = true;
> > > > 	dev->owner_pid = getpid();
> > > > 	ret = true;
> > > > out:
> > > > 	spin_unlock(dev->lock)
> > > > 	return ret;
> > > > }
> > > >
> > > >
> > > > bool rte_port_release_ownership(rte_eth_dev *dev) {
> > > >
> > > > 	boot ret = false;
> > > > 	spin_lock(dev->owner_lock);
> > > > 	if (!dev->locked)
> > > > 		goto out;
> > > > 	if (dev->owner_pid != getpid())
> > > > 		goto out;
> > > > 	dev->locked = false;
> > > > 	dev_owner_pid = 0;
> > > > 	ret = true;
> > > > out:
> > > > 	spin_unlock(dev->owner_lock)
> > > > 	return ret;
> > > > }
> > > >
> > > > bool rte_port_is_owned_by(struct rte_eth_dev *dev, pid_t pid) {
> > > > 	bool ret = false;
> > > >
> > > > 	spin_lock(dev->owner_lock);
> > > > 	if (pid)
> > > > 		ret = (dev->locked && (pid == dev->owner_pid));
> > > > 	else
> > > > 		ret = dev->locked;
> > > > 	spin_unlock(dev->owner_lock);
> > > > 	return ret;
> > > > }
> > > >
> > > > The idea here is that lock state is isolated from ownership
> > > > information.  Any context has the opportunity to lock the resource
> > > > (in this case the eth port) despite its ownership object.
> > > >
> > > > In comparison, your api, which is in may ways simmilar, separates
> > > > the creation of ownership objects to a separate api call, and that
> > > > ownership information embodies state that is integral to the ability
> > > > to get exclusive access to the resource.  I.E. if thread A calls
> > > > your owner_new call, and then thread B calls owner_new, thread A
> > > > will never be able to get access to any port unless it calls owner_new
> > again.
> > > >
> > > > Does that help clarify my position?
> > This would have been my understanding of what was being looked for too,
> > from my minimal understanding of the problem. Thanks for putting that
> > forward on behalf of many of us!
> > 
> > >
> > > Now I fully understand you, thanks for your patience.
> > >
> > > So, you are missing here one of the main ideas of my port ownership
> > intention.
> > > There are options for X>1 different uncoordinated owners running in the
> > same thread.
> > 
> > Thanks Matan for taking time to try and explain how your idea differs, but I
> > for one am still a little confused. Sorry for the late questions.
> > 
> > Sure, Neil's example above takes the pid or thread id as the owner id
> > parameter, but there is no reason we can't use the same scheme with
> > arbitrarily assigned owner ids, so long as they are unique. We can even have
> > a simple mapping table mapping ids to names of components.
> > >
> Sorry, don't understand your point here.
> My approach asked to allocate unique ID for "any part of code want to manage\use a port".
> What is the problem here and how do you suggest to fix it?
> 
> Neil approach (with process iD\ thread id ) is wrong because 2 different owners can run in same thread (as I explained a lot below).
> 
So, I may be wrong here, but it would be my opinion that the ownership record
should codify something about the owning context.  The fact that you want two
different owners to run in the context of the same thread is not a problem per
se, but rather an artifact of your adherence to the statement "any part of code
to manage/use a port".  I would assert that was perhaps a statement made in
error early during the design phase.  Perhaps it would be better to state that
any exectution context may take ownership of a port.

> > > For example:
> > > 1. Think about Testpmd control commands that call to failsafe port devop
> > which call to its sub-devices devops, while tespmd is different
> > owner(controlling failsafe-port) and failsafe is a different owner(controlling
> > all its sub-devices ports), There are both run control commands in the same
> > thread and there are uncoordinated!
Answered below.

> > >  2. Interrupt callbacks that anyone can register to them and all will run by
> > the DPDK host thread.
> > 
I'm sorry, I'm not clear on how your solution succededs here where my alternate
model fails.  Both models require co-ordination such that ownership of a port is
released and re-aquired by another thread, if I'm understanding this correctly.

> > Can you provide a little more details here: what is the specific issue or conflict
> > in each of these examples and how does your ownership proposal fix it,
> > when Neil's simpler approach doesn't?
> > 
> For the first example:
> My approach:
> Testpmd want to manage the fail-safe port, therefore it should allocate unique ID(only one time) and use owner set(by its ID) to take ownership of this port.
> If it succeed to take ownership it can manage the port.
> Failsafe PMD wants to manage its sub-devices ports and does the same process as Testpmd.
> Everything is ok.
> 
> Neil  approach:
> Testpmd want to manage the fail-safe port, therefore it just need to claim ownership(set) and its pid will take as the owner identifier.
> Failsafe PMD wants to manage its sub-devices ports and does the same process as Testpmd.
> But look these 2 entities run in same threads and there both can set the same pid. -> problem!
> 
I would argue thats not an error at all.  As above, the only thing wrong with
using the same ID to claim ownership of both ports is that it violates the
statement you referred to above, which I think is somewhat erroneous.  I would
further argue that using the same Id in both scenarios is preferable because it
accurately indicates the ownership relation between the top level failsafe
device and its slaves (i.e. that the application thread owns the failsafe
device, and transitively, the slaves).  There is no real need to codify the fact
that the failsafe port actually owns the slaves, above and beyond that statement
above.

There is a convienience to having ownership be differentiated in the
master/slave model when it comes to iterating over top level vs subordinate
ports, but I would agrue thats a problem that should be solved independently,
adding it here is somewhat confusing.  I would suggest adding a parent
rte_eth_Dev and childrent rte_eth_dev list to the rte_eth_dev structure so that
iterations can be preformed over top level devices, children, children of
children, etc.  You can do this with your ownership model as well of course, but
there are other ways to skin that cat.


> The second one just describe more scenario about more than one DPDK entities which run from the same thread.
> 
> > >
> > > So, no any optional  owner becomes an owner, it depends in the specific
> > implementation.
> > >
> > > So if some "part of code" wants to manage a port exclusively and wants to
> > take ownership of it to prevent other "part of code" to use this port :
> > > 1. Take ownership.
> > > 2. It should ask itself: Am I run in different threads\processes? If yes, it
> > should synchronize its port management.
> > > 3. Release ownership in the end.
> > >
> > > Remember that may be different "part of code"s running in the same
> > thread\threads\process\processes.
> > >
So it seems like the real point of contention that we need to settle here is,
what codifies an 'owner'.  Must it be a specific execution context, or can we
define any arbitrary section of code as being an owner?  I would agrue against
the latter.  While in your master/slave model I can see how it seems tempting, I
would suggest alternate use cases that make that ownership model ambiguous.  If,
for example, we use your interrupt example above, and an interrupt call back is
run for a given port, how, using your example, does it now which area of
code/object/thread to co-ordinate releasing of that port with so that it can
operate exclusively?


Thanks
Neil
> > > Thanks, Matan.
> > > >
> > > > Regards
> > > > Neil
> > > >
> > > > }
>