From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 1E1E01B1D8 for ; Sat, 20 Jan 2018 04:38:56 +0100 (CET) Received: from [107.15.66.59] (helo=[192.168.1.132]) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1ecjzT-00070G-HK; Fri, 19 Jan 2018 22:38:51 -0500 From: Tuxdriver To: Thomas Monjalon CC: , Matan Azrad , Bruce Richardson , "Ananyev, Konstantin" , Gaetan Rivet , "Wu, Jingjing" Date: Fri, 19 Jan 2018 22:38:45 -0500 Message-ID: <16111a5a988.2807.d241da8dbb85b87157d6a44ac288e71f@tuxdriver.com> In-Reply-To: <2018230.dXJnJTqQbo@xps> References: <20180118131017.GA1622@hmswarspite.think-freely.org> <1959306.0b6nHJGtEC@xps> <20180119194739.GF9519@hmswarspite.think-freely.org> <2018230.dXJnJTqQbo@xps> User-Agent: AquaMail/1.13.2-730 (build: 101300200) MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset="us-ascii" Content-Transfer-Encoding: 8bit X-Spam-Score: -2.9 (--) X-Spam-Status: No Subject: Re: [dpdk-dev] [PATCH v2 2/6] ethdev: add port ownership X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 20 Jan 2018 03:38:56 -0000 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 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 >