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 EAE171B2B8 for ; Thu, 18 Jan 2018 14:21:41 +0100 (CET) Received: from cpe-2606-a000-111b-4011-eaa3-4b92-4a68-8f24.dyn6.twc.com ([2606:a000:111b:4011:eaa3:4b92:4a68:8f24] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1ecA8G-0007UA-V6; Thu, 18 Jan 2018 08:21:32 -0500 Date: Thu, 18 Jan 2018 08:20:56 -0500 From: Neil Horman To: Matan Azrad Cc: "Ananyev, Konstantin" , Thomas Monjalon , Gaetan Rivet , "Wu, Jingjing" , "dev@dpdk.org" , "Richardson, Bruce" Message-ID: <20180118132056.GB1622@hmswarspite.think-freely.org> References: <2601191342CEEE43887BDE71AB9772588627DC25@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772588627DE30@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772588627E954@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772588627EE60@irsmsx105.ger.corp.intel.com> <20180117140020.GA5432@hmswarspite.think-freely.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.1 (2017-09-22) 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: Thu, 18 Jan 2018 13:21:42 -0000 On Wed, Jan 17, 2018 at 05:58:07PM +0000, Matan Azrad wrote: > > Hi Neil > > From: Neil Horman, Wednesday, January 17, 2018 4:00 PM > > On Wed, Jan 17, 2018 at 12:05:42PM +0000, Matan Azrad wrote: > > > > > > Hi Konstantin > > > From: Ananyev, Konstantin, Sent: Wednesday, January 17, 2018 1:24 PM > > > > Hi Matan, > > > > > > > > > Hi Konstantin > > > > > > > > > > From: Ananyev, Konstantin, Tuesday, January 16, 2018 9:11 PM > > > > > > Hi Matan, > > > > > > > > > > > > > > > > > > > > Hi Konstantin > > > > > > > From: Ananyev, Konstantin, Monday, January 15, 2018 8:44 PM > > > > > > > > Hi Matan, > > > > > > > > > Hi Konstantin > > > > > > > > > From: Ananyev, Konstantin, Monday, January 15, 2018 1:45 > > > > > > > > > PM > > > > > > > > > > Hi Matan, > > > > > > > > > > > Hi Konstantin > > > > > > > > > > > From: Ananyev, Konstantin, Friday, January 12, 2018 > > > > > > > > > > > 2:02 AM > > > > > > > > > > > > Hi Matan, > > > > > > > > > > > > > Hi Konstantin > > > > > > > > > > > > > From: Ananyev, Konstantin, Thursday, January 11, > > > > > > > > > > > > > 2018 > > > > > > > > > > > > > 2:40 PM > > > > > > > > > > > > > > Hi Matan, > > > > > > > > > > > > > > > Hi Konstantin > > > > > > > > > > > > > > > From: Ananyev, Konstantin, Wednesday, January > > > > > > > > > > > > > > > 10, > > > > > > > > > > > > > > > 2018 > > > > > > > > > > > > > > > 3:36 PM > > > > > > > > > > > > > > > > Hi Matan, > > > > > > > > > > > > > > > > > > > > > > > It is good to see that now scanning/updating > > > > > > > > > > > > > > > > rte_eth_dev_data[] is lock protected, but it > > > > > > > > > > > > > > > > might be not very plausible to protect both > > > > > > > > > > > > > > > > data[] and next_owner_id using the > > > > > > > > > > same lock. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I guess you mean to the owner structure in > > > > > > > > > > rte_eth_dev_data[port_id]. > > > > > > > > > > > > > > > The next_owner_id is read by ownership > > > > > > > > > > > > > > > APIs(for owner validation), so it > > > > > > > > > > > > > > makes sense to use the same lock. > > > > > > > > > > > > > > > Actually, why not? > > > > > > > > > > > > > > > > > > > > > > > > > > > > Well to me next_owner_id and rte_eth_dev_data[] > > > > > > > > > > > > > > are not directly > > > > > > > > > > > > related. > > > > > > > > > > > > > > You may create new owner_id but it doesn't mean > > > > > > > > > > > > > > you would update rte_eth_dev_data[] immediately. > > > > > > > > > > > > > > And visa-versa - you might just want to update > > > > > > > > > > > > > > rte_eth_dev_data[].name or .owner_id. > > > > > > > > > > > > > > It is not very good coding practice to use same > > > > > > > > > > > > > > lock for non-related data structures. > > > > > > > > > > > > > > > > > > > > > > > > > > > I see the relation like next: > > > > > > > > > > > > > Since the ownership mechanism synchronization is > > > > > > > > > > > > > in ethdev responsibility, we must protect against > > > > > > > > > > > > > user mistakes as much as we can by > > > > > > > > > > > > using the same lock. > > > > > > > > > > > > > So, if user try to set by invalid owner (exactly > > > > > > > > > > > > > the ID which currently is > > > > > > > > > > > > allocated) we can protect on it. > > > > > > > > > > > > > > > > > > > > > > > > Hmm, not sure why you can't do same checking with > > > > > > > > > > > > different lock or atomic variable? > > > > > > > > > > > > > > > > > > > > > > > The set ownership API is protected by ownership lock > > > > > > > > > > > and checks the owner ID validity By reading the next owner > > ID. > > > > > > > > > > > So, the owner ID allocation and set API should use the > > > > > > > > > > > same atomic > > > > > > > > > > mechanism. > > > > > > > > > > > > > > > > > > > > Sure but all you are doing for checking validity, is > > > > > > > > > > check that owner_id > 0 &&& owner_id < next_ownwe_id, > > right? > > > > > > > > > > As you don't allow owner_id overlap (16/3248 bits) you > > > > > > > > > > can safely do same check with just > > atomic_get(&next_owner_id). > > > > > > > > > > > > > > > > > > > It will not protect it, scenario: > > > > > > > > > - current next_id is X. > > > > > > > > > - call set ownership of port A with owner id X by thread > > > > > > > > > 0(by user > > > > > > mistake). > > > > > > > > > - context switch > > > > > > > > > - allocate new id by thread 1 and get X and change next_id > > > > > > > > > to > > > > > > > > > X+1 > > > > > > > > atomically. > > > > > > > > > - context switch > > > > > > > > > - Thread 0 validate X by atomic_read and succeed to take > > > > ownership. > > > > > > > > > - The system loosed the port(or will be managed by two > > > > > > > > > entities) - > > > > > > crash. > > > > > > > > > > > > > > > > > > > > > > > > Ok, and how using lock will protect you with such scenario? > > > > > > > > > > > > > > The owner set API validation by thread 0 should fail because > > > > > > > the owner > > > > > > validation is included in the protected section. > > > > > > > > > > > > Then your validation function would fail even if you'll use > > > > > > atomic ops instead of lock. > > > > > No. > > > > > With atomic this specific scenario will cause the validation to pass. > > > > > > > > Can you explain to me how? > > > > > > > > rte_eth_is_valid_owner_id(uint16_t owner_id) { > > > > int32_t cur_owner_id = > > > > RTE_MIN(rte_atomic32_get(next_owner_id), > > > > UINT16_MAX); > > > > > > > > if (owner_id == RTE_ETH_DEV_NO_OWNER || owner > > > > > cur_owner_id) { > > > > RTE_LOG(ERR, EAL, "Invalid owner_id=%d.\n", owner_id); > > > > return 0; > > > > } > > > > return 1; > > > > } > > > > > > > > Let say your next_owne_id==X, and you invoke > > > > rte_eth_is_valid_owner_id(owner_id=X+1) - it would fail. > > > > > > Explanation: > > > The scenario with locks: > > > next_owner_id = X. > > > Thread 0 call to set API(with invalid owner Y=X) and take lock. > > > Context switch. > > > Thread 1 call to owner_new and stuck in the lock. > > > Context switch. > > > Thread 0 does owner id validation and failed(Y>=X) - unlock the lock and > > return failure to the user. > > > Context switch. > > > Thread 1 take the lock and update X to X+1, then, unlock the lock. > > > Everything is OK! > > > > > > The same scenario with atomics: > > > next_owner_id = X. > > > Thread 0 call to set API(with invalid owner Y=X) and take lock. > > > Context switch. > > > Thread 1 call to owner_new and change X to X+1(atomically). > > > Context switch. > > > Thread 0 does owner id validation and success(Y<(atomic)X+1) - unlock the > > lock and return success to the user. > > > Problem! > > > > > > > > > Matan is correct here, there is no way to preform parallel set operations > > using just and atomic variable here, because multiple reads of > > next_owner_id need to be preformed while it is stable. That is to say > > rte_eth_next_owner_id must be compared to RTE_ETH_DEV_NO_OWNER > > and owner_id in rte_eth_is_valid_owner_id. If you were to only use an > > atomic_read on such a variable, it could be incremented by the owner_new > > function between the checks and an invalid owner value could become valid > > because a third thread incremented the next value. The state of > > next_owner_id must be kept stable during any validity checks > > > > That said, I really have to wonder why ownership ids are really needed here > > at all. It seems this design could be much simpler with the addition of a per- > > port lock (and optional ownership record). The API could consist of three > > operations: > > > > ownership_set > > ownership_tryset > > ownership_release > > ownership_get > > > > > > The first call simply tries to take the per-port lock (blocking if its already > > locked) > > > > Per port lock is not good because the ownership mechanism must to be synchronized with the port creation\release. > So the port creation and port ownership should use the same lock. > In what way do you need to synchronize with port creation? If a port has not yet been created, then by definition the owner must be the thread calling the create function. If you are concerned about the mechanics of the port data structure (i.e. the fact that rte_eth_devices is statically allocated, you can add a lock structure to the rte_eth_dev struct and initialize it statically with RTE_SPINLOCK_INITAIZER() > I didn't find precedence for blocking function in ethdev. > Then perhaps we don't need that api call. Perhaps ownership_tryset is enough. > > The second call is a non-blocking version of the first > > > > The third unlocks the port, allowing others to take ownership > > > > The fourth returns whatever ownership record you want to encode with the > > lock. > > > > The addition of all this id checking seems a bit overcomplicated > > You miss the identification of the owner - we want to allow info of the owner for printing and easy debug. > And it is makes sense to manage the owner uniqueness by unique ID. > I specifically pointed that out above. There is no reason an owernship record couldn't be added to the rte_eth_dev structure. > The API already discussed a lot in the previous version, Do you really want, now, to open it again? > What I want is the most useful and elegant ownership API available. If you think what you have is that, so be it. I only bring this up because the amount of debate you and Konstantin have had over lock safety causes me to wonder if this isn't an overly complex design. Neil > > Neil > >