From: Tuxdriver <nhorman@tuxdriver.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: <dev@dpdk.org>, Matan Azrad <matan@mellanox.com>,
Bruce Richardson <bruce.richardson@intel.com>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
Gaetan Rivet <gaetan.rivet@6wind.com>,
"Wu, Jingjing" <jingjing.wu@intel.com>
Subject: Re: [dpdk-dev] [PATCH v2 2/6] ethdev: add port ownership
Date: Fri, 19 Jan 2018 22:38:45 -0500 [thread overview]
Message-ID: <16111a5a988.2807.d241da8dbb85b87157d6a44ac288e71f@tuxdriver.com> (raw)
In-Reply-To: <2018230.dXJnJTqQbo@xps>
Writing from my phone, so sorry for typos and top posting.
Need to apologise for a misunderstanding on my part. I had a dyslexic
moment and reversed the validity check on the port owner comparison. What
I thought was => was actually =<, and so my concern that only the last
allocated owner is false, and erroneous on my part.
More comments as I'm able while afk
Neil
Sent with AquaMail for Android
http://www.aqua-mail.com
On January 19, 2018 3:20:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> 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
>
next prev parent reply other threads:[~2018-01-20 3:38 UTC|newest]
Thread overview: 212+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-28 11:57 [dpdk-dev] [PATCH 0/5] ethdev: Port ownership Matan Azrad
2017-11-28 11:57 ` [dpdk-dev] [PATCH 1/5] ethdev: free a port by a dedicated API Matan Azrad
2017-11-28 11:57 ` [dpdk-dev] [PATCH 2/5] ethdev: add port ownership Matan Azrad
2017-11-30 12:36 ` Neil Horman
2017-11-30 13:24 ` Gaëtan Rivet
2017-11-30 14:30 ` Matan Azrad
2017-11-30 15:09 ` Gaëtan Rivet
2017-11-30 15:43 ` Matan Azrad
2017-12-01 12:09 ` Neil Horman
2017-12-03 8:04 ` Matan Azrad
2017-12-03 11:10 ` Ananyev, Konstantin
2017-12-03 13:46 ` Matan Azrad
2017-12-04 16:01 ` Neil Horman
2017-12-04 18:10 ` Matan Azrad
2017-12-04 22:30 ` Neil Horman
2017-12-05 6:08 ` Matan Azrad
2017-12-05 10:05 ` Bruce Richardson
2017-12-08 11:35 ` Thomas Monjalon
2017-12-08 12:31 ` Neil Horman
2017-12-21 17:06 ` Thomas Monjalon
2017-12-21 17:43 ` Neil Horman
2017-12-21 19:37 ` Matan Azrad
2017-12-21 20:14 ` Neil Horman
2017-12-21 21:57 ` Matan Azrad
2017-12-22 14:26 ` Neil Horman
2017-12-23 22:36 ` Matan Azrad
2017-12-29 16:56 ` Neil Horman
2017-12-05 19:26 ` Neil Horman
2017-12-08 11:06 ` Thomas Monjalon
2017-12-05 11:12 ` Ananyev, Konstantin
2017-12-05 11:44 ` Ananyev, Konstantin
2017-12-05 11:53 ` Thomas Monjalon
2017-12-05 14:56 ` Bruce Richardson
2017-12-05 14:57 ` Ananyev, Konstantin
2017-12-05 11:47 ` Thomas Monjalon
2017-12-05 15:13 ` Ananyev, Konstantin
2017-12-05 15:49 ` Thomas Monjalon
2017-11-28 11:57 ` [dpdk-dev] [PATCH 3/5] net/failsafe: free an eth port by a dedicated API Matan Azrad
2017-11-28 11:58 ` [dpdk-dev] [PATCH 4/5] net/failsafe: use ownership mechanism to own ports Matan Azrad
2017-11-28 11:58 ` [dpdk-dev] [PATCH 5/5] app/testpmd: adjust ethdev port ownership Matan Azrad
2018-01-07 9:45 ` [dpdk-dev] [PATCH v2 0/6] ethdev: " Matan Azrad
2018-01-07 9:45 ` [dpdk-dev] [PATCH v2 1/6] ethdev: fix port data reset timing Matan Azrad
2018-01-07 9:45 ` [dpdk-dev] [PATCH v2 2/6] ethdev: add port ownership Matan Azrad
2018-01-10 13:36 ` Ananyev, Konstantin
2018-01-10 16:58 ` Matan Azrad
2018-01-11 12:40 ` Ananyev, Konstantin
2018-01-11 14:51 ` Matan Azrad
2018-01-12 0:02 ` Ananyev, Konstantin
2018-01-12 7:24 ` Matan Azrad
2018-01-15 11:45 ` Ananyev, Konstantin
2018-01-15 13:09 ` Matan Azrad
2018-01-15 18:43 ` Ananyev, Konstantin
2018-01-16 8:04 ` Matan Azrad
2018-01-16 19:11 ` Ananyev, Konstantin
2018-01-16 20:32 ` Matan Azrad
2018-01-17 11:24 ` Ananyev, Konstantin
2018-01-17 12:05 ` Matan Azrad
2018-01-17 12:54 ` Ananyev, Konstantin
2018-01-17 13:10 ` Matan Azrad
2018-01-17 16:52 ` Ananyev, Konstantin
2018-01-17 18:02 ` Matan Azrad
2018-01-17 20:34 ` Matan Azrad
2018-01-18 14:17 ` Ananyev, Konstantin
2018-01-18 14:26 ` Matan Azrad
2018-01-18 14:41 ` Ananyev, Konstantin
2018-01-18 14:45 ` Matan Azrad
2018-01-18 14:51 ` Ananyev, Konstantin
2018-01-18 15:00 ` Matan Azrad
2018-01-17 14:00 ` Neil Horman
2018-01-17 17:01 ` Ananyev, Konstantin
2018-01-18 13:10 ` Neil Horman
2018-01-18 14:00 ` Matan Azrad
2018-01-18 16:54 ` Neil Horman
2018-01-18 17:20 ` Matan Azrad
2018-01-18 18:41 ` Neil Horman
2018-01-18 20:21 ` Matan Azrad
2018-01-19 1:41 ` Neil Horman
2018-01-19 7:14 ` Matan Azrad
2018-01-19 9:30 ` Bruce Richardson
2018-01-19 10:44 ` Matan Azrad
2018-01-19 13:30 ` Neil Horman
2018-01-19 13:57 ` Matan Azrad
2018-01-19 14:13 ` Thomas Monjalon
2018-01-19 15:27 ` Neil Horman
2018-01-19 17:17 ` Thomas Monjalon
2018-01-19 17:43 ` Neil Horman
2018-01-19 18:12 ` Thomas Monjalon
2018-01-19 19:47 ` Neil Horman
2018-01-19 20:19 ` Thomas Monjalon
2018-01-19 22:52 ` Neil Horman
2018-01-20 3:38 ` Tuxdriver [this message]
2018-01-20 12:54 ` Ananyev, Konstantin
2018-01-20 14:02 ` Thomas Monjalon
2018-01-19 12:55 ` Neil Horman
2018-01-19 13:52 ` Neil Horman
2018-01-18 16:27 ` Neil Horman
2018-01-17 17:58 ` Matan Azrad
2018-01-18 13:20 ` Neil Horman
2018-01-18 14:52 ` Matan Azrad
2018-01-19 13:57 ` Neil Horman
2018-01-19 14:07 ` Thomas Monjalon
2018-01-19 14:32 ` Neil Horman
2018-01-19 17:09 ` Thomas Monjalon
2018-01-19 17:37 ` Neil Horman
2018-01-19 18:10 ` Thomas Monjalon
2018-01-21 22:12 ` Ferruh Yigit
2018-01-07 9:45 ` [dpdk-dev] [PATCH v2 3/6] ethdev: synchronize port allocation Matan Azrad
2018-01-07 9:58 ` Matan Azrad
2018-01-07 9:45 ` [dpdk-dev] [PATCH v2 4/6] net/failsafe: free an eth port by a dedicated API Matan Azrad
2018-01-07 9:45 ` [dpdk-dev] [PATCH v2 5/6] net/failsafe: use ownership mechanism to own ports Matan Azrad
2018-01-08 10:32 ` Gaëtan Rivet
2018-01-08 11:16 ` Matan Azrad
2018-01-08 11:35 ` Gaëtan Rivet
2018-01-07 9:45 ` [dpdk-dev] [PATCH v2 6/6] app/testpmd: adjust ethdev port ownership Matan Azrad
2018-01-08 11:39 ` Gaëtan Rivet
2018-01-08 12:30 ` Matan Azrad
2018-01-08 13:30 ` Gaëtan Rivet
2018-01-08 13:55 ` Matan Azrad
2018-01-08 14:21 ` Gaëtan Rivet
2018-01-08 14:42 ` Matan Azrad
2018-01-16 5:53 ` Lu, Wenzhuo
2018-01-16 8:15 ` Matan Azrad
2018-01-17 0:46 ` Lu, Wenzhuo
2018-01-17 8:51 ` Matan Azrad
2018-01-18 0:53 ` Lu, Wenzhuo
2018-01-18 16:35 ` [dpdk-dev] [PATCH v3 0/7] Port ownership and syncronization Matan Azrad
2018-01-18 16:35 ` [dpdk-dev] [PATCH v3 1/7] ethdev: fix port data reset timing Matan Azrad
2018-01-18 17:00 ` Thomas Monjalon
2018-01-19 12:38 ` Ananyev, Konstantin
2018-03-05 11:24 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
2018-03-05 14:52 ` Matan Azrad
2018-03-05 15:06 ` Ferruh Yigit
2018-03-05 15:12 ` Matan Azrad
2018-03-27 22:37 ` Ferruh Yigit
2018-03-28 12:07 ` Matan Azrad
2018-03-30 10:39 ` Ferruh Yigit
2018-04-19 11:07 ` Ferruh Yigit
2018-04-25 12:16 ` Matan Azrad
2018-04-25 12:30 ` Ori Kam
2018-04-25 12:54 ` Ferruh Yigit
2018-04-25 14:01 ` Matan Azrad
2018-01-18 16:35 ` [dpdk-dev] [PATCH v3 2/7] ethdev: fix used portid allocation Matan Azrad
2018-01-18 17:00 ` Thomas Monjalon
2018-01-19 12:40 ` Ananyev, Konstantin
2018-01-20 16:48 ` Matan Azrad
2018-01-20 17:26 ` Ananyev, Konstantin
2018-01-18 16:35 ` [dpdk-dev] [PATCH v3 3/7] ethdev: add port ownership Matan Azrad
2018-01-18 21:11 ` Thomas Monjalon
2018-01-19 12:41 ` Ananyev, Konstantin
2018-01-18 16:35 ` [dpdk-dev] [PATCH v3 4/7] ethdev: synchronize port allocation Matan Azrad
2018-01-18 20:43 ` Thomas Monjalon
2018-01-18 20:52 ` Matan Azrad
2018-01-18 21:17 ` Thomas Monjalon
2018-01-19 12:47 ` Ananyev, Konstantin
2018-01-18 16:35 ` [dpdk-dev] [PATCH v3 5/7] net/failsafe: free an eth port by a dedicated API Matan Azrad
2018-01-18 16:35 ` [dpdk-dev] [PATCH v3 6/7] net/failsafe: use ownership mechanism to own ports Matan Azrad
2018-01-18 16:35 ` [dpdk-dev] [PATCH v3 7/7] app/testpmd: adjust ethdev port ownership Matan Azrad
2018-01-19 12:37 ` Ananyev, Konstantin
2018-01-19 12:51 ` Matan Azrad
2018-01-19 13:08 ` Ananyev, Konstantin
2018-01-19 13:35 ` Matan Azrad
2018-01-19 15:00 ` Gaëtan Rivet
2018-01-20 18:14 ` Matan Azrad
2018-01-22 10:17 ` Gaëtan Rivet
2018-01-22 11:22 ` Matan Azrad
2018-01-22 12:28 ` Ananyev, Konstantin
2018-01-22 13:22 ` Matan Azrad
2018-01-22 20:48 ` Ananyev, Konstantin
2018-01-23 8:54 ` Matan Azrad
2018-01-23 12:56 ` Gaëtan Rivet
2018-01-23 14:30 ` Matan Azrad
2018-01-25 9:36 ` Matan Azrad
2018-01-25 10:05 ` Thomas Monjalon
2018-01-25 11:15 ` Ananyev, Konstantin
2018-01-25 11:33 ` Thomas Monjalon
2018-01-25 11:55 ` Ananyev, Konstantin
2018-01-23 13:34 ` Ananyev, Konstantin
2018-01-23 14:18 ` Thomas Monjalon
2018-01-23 15:12 ` Ananyev, Konstantin
2018-01-23 15:18 ` Ananyev, Konstantin
2018-01-23 17:33 ` Thomas Monjalon
2018-01-23 21:18 ` Ananyev, Konstantin
2018-01-24 8:10 ` Thomas Monjalon
2018-01-24 18:30 ` Ananyev, Konstantin
2018-01-25 10:55 ` Thomas Monjalon
2018-01-25 11:09 ` Ananyev, Konstantin
2018-01-25 11:27 ` Thomas Monjalon
2018-01-23 14:43 ` Matan Azrad
2018-01-20 21:24 ` [dpdk-dev] [PATCH v4 0/7] Port ownership and syncronization Matan Azrad
2018-01-20 21:24 ` [dpdk-dev] [PATCH v4 1/7] ethdev: fix port data reset timing Matan Azrad
2018-01-20 21:24 ` [dpdk-dev] [PATCH v4 2/7] ethdev: fix used portid allocation Matan Azrad
2018-01-20 21:24 ` [dpdk-dev] [PATCH v4 3/7] ethdev: add port ownership Matan Azrad
2018-01-21 20:43 ` Ferruh Yigit
2018-01-21 20:46 ` Ferruh Yigit
2018-01-20 21:24 ` [dpdk-dev] [PATCH v4 4/7] ethdev: synchronize port allocation Matan Azrad
2018-01-20 21:24 ` [dpdk-dev] [PATCH v4 5/7] net/failsafe: free an eth port by a dedicated API Matan Azrad
2018-01-20 21:24 ` [dpdk-dev] [PATCH v4 6/7] net/failsafe: use ownership mechanism to own ports Matan Azrad
2018-01-20 21:24 ` [dpdk-dev] [PATCH v4 7/7] app/testpmd: adjust ethdev port ownership Matan Azrad
2018-01-22 16:38 ` [dpdk-dev] [PATCH v5 0/7] Port ownership and synchronization Matan Azrad
2018-01-22 16:38 ` [dpdk-dev] [PATCH v5 1/7] ethdev: fix port data reset timing Matan Azrad
2018-01-22 16:38 ` [dpdk-dev] [PATCH v5 2/7] ethdev: fix used portid allocation Matan Azrad
2018-01-22 16:38 ` [dpdk-dev] [PATCH v5 3/7] ethdev: add port ownership Matan Azrad
2018-01-22 16:38 ` [dpdk-dev] [PATCH v5 4/7] ethdev: synchronize port allocation Matan Azrad
2018-01-22 16:38 ` [dpdk-dev] [PATCH v5 5/7] net/failsafe: free an eth port by a dedicated API Matan Azrad
2018-01-22 16:38 ` [dpdk-dev] [PATCH v5 6/7] net/failsafe: use ownership mechanism to own ports Matan Azrad
2018-01-22 16:38 ` [dpdk-dev] [PATCH v5 7/7] app/testpmd: adjust ethdev port ownership Matan Azrad
2018-01-25 1:47 ` Lu, Wenzhuo
2018-01-25 8:30 ` Matan Azrad
2018-01-26 0:50 ` Lu, Wenzhuo
2018-01-29 11:21 ` [dpdk-dev] [PATCH v5 0/7] Port ownership and synchronization Matan Azrad
2018-01-31 19:53 ` Thomas Monjalon
2018-01-25 14:35 ` [dpdk-dev] [PATCH v3 0/7] Port ownership and syncronization Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=16111a5a988.2807.d241da8dbb85b87157d6a44ac288e71f@tuxdriver.com \
--to=nhorman@tuxdriver.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=gaetan.rivet@6wind.com \
--cc=jingjing.wu@intel.com \
--cc=konstantin.ananyev@intel.com \
--cc=matan@mellanox.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).