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

Neil Horman nhorman at tuxdriver.com
Tue Dec 5 20:26:40 CET 2017


On Tue, Dec 05, 2017 at 06:08:35AM +0000, Matan Azrad wrote:
><snip>
> Please look at the code again, secondary process cannot take ownership, I don't allow it!
> Actually, I think it must not be like that as no limitation for that in any other ethdev configurations.
> 
Sure you do.  Consider the following situation, two tasks, A and B running
independently on separate CPUS:

TASK A					TASK B
================================================================================
calls rte_eth_dev_owner_new (gets id 2)| calls rte_eth_dev_owner_new (gets id 1)
				       |
calls rte_eth_dev_owner_set on port 1  | calls rte_eth_dev_owner_set (port 1)
				       |
sets port_owner->id = 2		       | gets removed from cpu via scheduler
				       |
				       | returns to continue running on cpu
				       |
Gets interrupted immediately before    | completes rte_eth_dev_owner_set, 
 return 0 statement		       |  setting port_owner->id = 1
				       |
				       | returns 0 from rte_eth_dev_owner_set
is scheduled back on the cpu	       |
				       |
returns 0 from rte_eth_dev_owner_set   |

in the above scenario, you've allowed two tasks to race through the ownership
set routine, and while your intended semantics indicate that task A should have
taken ownership of the port, task B actually did, and whats worse, both tasks
think they completed successfully.

I get that much of dpdk relies on the fact that the application either handles
all the locking, or architects itself so that a single thread of execution (or
at least only one thread at a time), is responsible for packet processing and
port configuration. If you are assuming the former, you've done a disservice to
the downstream consumer, because the locking is the intricate part of this
operation (i.e. you are requiring that the developer figure out what granularity
of locking is required such that you don't serialize too many operations that
don't need it, while maintaining enough serialization that you don't corrupt the
data that they wanted the api to manage in the first place.  If you assert that
the application should only be using a single task to do these operations to
begin with, then this API isn't overly useful, because theres only one thread
pushing data into the library and, by definition it implicitly owns all the
ports, or at least knows which ports it shouldn't mess with (e.g subordunate
ports in a failsafe device).

> > > Any port configuration (for example port creation and release) is not
> > internally synchronized between the primary to secondary processes so I
> > don't see any reason to synchronize port ownership.
> > Yes, and I've never agreed with that design point either, because the fact of
> > the matter is that one of two things must be true in relation to port
> > configuration:
> > 
> > 1) Either multiple processes will attempt to read/change port
> > configuration/ownership
> > 
> > or
> > 
> > 2) port ownership is implicitly given to a single task (be it a primary or
> > secondary task), and said ownership is therefore implicitly known by the
> > application
> > 
> > Either situation may be true depending on the details of the application being
> > built, but regardless, if (2) is true, then this api isn't really needed at all,
> > because the application implicitly has been designed to have a port be
> > owned by a given task. 
> 
> Here I think you miss something, the port ownership is not mainly for process DPDK entities,
> The entity can be also PMD, library, application in same process.
> For MP it is nice to have, the secondary just can see the primary owners and take decision accordingly (please read my answer to Konstatin above). 
> 
But the former is just a case of the latter (in fact worse).  if you expect
another execution context out of the control of the application to query this
API, then you are in an MP situation, and definately need to provide mutually
exclusive access to your data.  If instead you expect all other execution
contexts to suspend (or otherwise refrain from accessing this API) while a
single task makes changes, then by definition you have already had to create
some syncrnoization between those contexts and are capable of informing them of
of the new ownership scheme

The bottom line is, either you expect multiple access, or you dont.  If you
expect multiple access, and you belive that said access are not completely under
the control of your application, you need to protect those accesses against one
another.  If you don't expect multiple access (or expect your application to
architect itself to enforce single access), then you've created a environment in
which the single accessor already has to know all the information regarding port
ownership that you store in the API.

>  If (1) is true, then all the locking required to access
> > the data of port ownership needs to be adhered to.
> > 
> 
> And all the port configurations!
> I think it is behind to this thread.
> 
> 
> > I understand that you are taking Thomas' words to mean that all paths are
> > lockless in the DPDK, and that is true as a statement of fact (in that the DPDK
> > doesn't synchronize access to internal data).  What it does do, is leave that
> > locking as an exercize for the downstream consumer of the library.  While I
> > don't agree with it, I can see that such an argument is valid for hot paths such
> > as transmission and reception where you may perhaps want to minimize
> > your locking by assigning a single task to do that work, but port configuration
> > and ownership isn't a hot path.  If you mean to allow potentially multiple
> > tasks to access configuration (including port ownership), be it frequently or
> > just occasionaly, why are you making a downstream developer recognize the
> > need for locking here outside the library?  It just seems like very bad general
> > practice to me.
> > 
> > > If the primary-secondary process want to manage(configure) same port in
> > same time, they must be synchronized by the applications, so this is the case
> > in port ownership too (actually I don't think this synchronization is realistic
> > because many configurations of the port are not in shared memory).
> > Yes, it is the case, my question is, why?  We're not talking about a time
> > sensitive execution path here.  And by avoiding it you're just making work
> > that has to be repeated by every downstream consumer.
> 
> I think you suggest to make all the ethdev configuration race safe, it is behind to this thread.
> Current ethdev implementation leave the race management to applications, so port ownership as any other port configurations should be designed in the same method.
> 
I would like to make ethdev configuration race safe, but thats a fight I've
lost, but I strongly disagree that just because some parts of the dpdk leave
race safety to the user, doesn't mean you have to.  Its just silly not to here.
We're not talking about a hot execution path here, we're talking about one time
/ rare configuration changes.  To insert a lock (or other lightweight atomic
operation) costs nothing in terms of execution time, and saves downstream
consumers significant time figuring out what the best mutual exclusion strategy
is.  Why wouldn't you do that?

Neil

> > 
> > Neil
> 
> 


More information about the dev mailing list