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

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Jan 19, 2018 at 09:19:18PM +0100, Thomas Monjalon wrote:
Apolgies for the top post, but I'm preparing for a trip out of the country, and
so may not have time to fully answer these questions until I get back (or at
least until I get someplace with power and internet).  If the conversation is
still going at that time, I'll chime back in
Neil

> 19/01/2018 20:47, Neil Horman:
> > On Fri, Jan 19, 2018 at 07:12:36PM +0100, Thomas Monjalon wrote:
> > > 19/01/2018 18:43, Neil Horman:
> > > > On Fri, Jan 19, 2018 at 06:17:51PM +0100, Thomas Monjalon wrote:
> > > > > 19/01/2018 16:27, Neil Horman:
> > > > > > On Fri, Jan 19, 2018 at 03:13:47PM +0100, Thomas Monjalon wrote:
> > > > > > > 19/01/2018 14:30, Neil Horman:
> > > > > > > > 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.
> > > > > > > 
> > > > > > > This is the first thing explained in the cover letter:
> > > > > > > "2. The port usage synchronization will be managed by the port owner."
> > > > > > > There is no intent to manage the threads synchronization for a given port.
> > > > > > > It is the responsibility of the owner (a code object) to configure its
> > > > > > > port via only one thread.
> > > > > > > It is consistent with not trying to manage threads synchronization
> > > > > > > for Rx/Tx on a given queue.
> > > > > > > 
> > > > > > > 
> > > > > > Yes, in his cover letter, and I contend that notion is an invalid design point.
> > > > > > By codifying an area of code as an 'owner', rather than an execution context,
> > > > > > you're defining the notion of heirarchy, not ownership. That is to say,
> > > > > > you want to codify the notion that there are top level ports that the
> > > > > > application might see, and some of those top level ports are parents to
> > > > > > subordinate ports, which only the parent port should access directly.  If thats
> > > > > > all you want to encode, there are far easier ways to do it:
> > > > > > 
> > > > > > struct rte_eth_shared_data {
> > > > > > 	< existing bits >
> > > > > > 	struct rte_eth_port_list {
> > > > > > 		struct rte_eth_port_list *children;
> > > > > > 		struct rte_eth_port_list *parent;
> > > > > > 	};
> > > > > > };
> > > > > > 
> > > > > > 
> > > > > > Build an api around a structure like that, so that the parent/child relationship
> > > > > > is globally clear, and this would be much easier, especially if you want to
> > > > > > continue asserting that the notion of synchronization/exclusion is an exercise
> > > > > > left to the application.
> > > > > 
> > > > > Not only Neil.
> > > > > An owner can be something else than a port.
> > > > > An owner can be an app process (multi-processes).
> > > > > An owner can be a library.
> > > > > The intent is really to solve the generic problem of which code
> > > > > is managing a port.
> > > > > 
> > > > I don't see how this precludes any part of what you just said.  Define the
> > > > rte_eth_port_list externally to the shared_data struct and allow any object you
> > > > want to allocate it, then anything you want to control a heirarchy of ports can
> > > > do so without issue, and the structure is far more clear than an opaque id that
> > > > carries subtle semantic ordering with it.
> > > 
> > > Sorry, I don't understand. Please could you rephrase?
> > > 
> > 
> > Sure, I'm saying the fact that you want an owner to be an object
> > (library/port/process) rather than strictly an execution context
> > (process/thread) doesn't preclude what I'm proposing above.  You can create a
> > generic version of the strcture I propose above like so:
> > 
> > struct rte_obj_heirarchy {
> > 	struct rte_obj_heirarchy *children;
> > 	struct rte_obj_heirarchy *parent;
> > 	void *owner_data; /* optional */
> > };
> > 
> > And embed that structure in any object you would like to give a representative
> > heirarchy to, you then have a fairly simple api
> > 
> > struct rte_obj_heirarchy *heirarchy_alloc();
> > bool heirarchy_set(struct rte_obj_heirarchy *parent, struct rte_obj_heirarcy *child)
> > void heirarchy_release(struct rte_obj_heirarchy *obj)
> > 
> > That gives you the privately held list relationship I think you are in part
> > looking for (i.e. the ability for a failsafe device to iterate over the ports it
> > is in control of), without the awkwardness of the ordinal priority that the
> > current implementation imposes.
> 
> What is the awkward ordinal priority?
> I see you discuss it below. So let's discuss it below.
> 
> > In summary, if what you want is ownership in the strictest sense of the word
> > (i.e. mutually exclusive access, which I think makes sense), then using a lock
> > and flag is really the simplest way to go.  If instead what you want is a
> > heirarchical relationship where you can iterate over a limited set of objects
> > (the failsafe child port example), then the above is what you want.
> 
> We want only ownership. That's why it's called ownership :)
> The hierarchical relationship is private to the owner.
> For instance, failsafe implements its own list of sub-devices.
> So we just need to expose that the ports are already owned.
> 
> > The soution Matan is providing does some of each of these things, but comes with
> > very odd side effects
> > 
> > It offers a level of mutual exclusion, in that only one
> > object can own another at a time, but does so in a way that introduces this very
> > atypical ordinality (once an ownership object is created with owner_new, any
> > previously created ownership object will be denied the ability to take ownership
> > of a port)
> 
> You mean only the last owner id can take an ownership?
> If yes, it looks like a bug.
> Please could you show what is responsible of this effect in the patch?
> 
> > It also offers a level of filtering (in that if you can set the ownership id of
> > a given set of object to the value X, you can then iterate over them by
> > iterating over all objects of that type, and filtering on their id), but it
> > offers no clear in-memory relationship between parent and children (i.e. if you
> > were to look at at an object in a debugger and see that it was owned by owner id
> > X, it would provide you with no indicator of what object held the allocated
> > ownership object assigned id X.
> 
> I think it is wrong. There is an owner name for debug/printing purpose.
> 
> > My proposal trades a few bytes of data in
> > exchage for a global clear, definitive heirarcy relationship.  And if you add an
> > api call and a spinlock, you can easily graft on mutual exclusion here, by
> > blocking access to objects that arent the immediate parent of a given object.
> 
> For the hierarchical relationship, I think it is over-engineered.
> For blocking access, it means you need a caller id parameter in every
> functions in order to identify if the caller is the owner.
> 
> My summary:
> - you think there is a bug - needs to show
> - you think about relationship needs that I don't see
> - you think about access permission which would be a huge change
>