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
