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

[Bruce Richardson]

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.
> 
> 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!
>  2. Interrupt callbacks that anyone can register to them and all will run by the DPDK host thread.

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?

> 
> 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.
> 
> Thanks, Matan.
> > 
> > Regards
> > Neil
> > 
> > }