From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-he1eur01on0042.outbound.protection.outlook.com [104.47.0.42]) by dpdk.org (Postfix) with ESMTP id BC4D7A48C for ; Fri, 12 Jan 2018 08:24:07 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=nD1jy6QTmUU2nYiOLrKlI/GHNZgnJnGgaJprkSleIQg=; b=NGZeOzkWzLWn/bv1tvXyVIcOzfOIZxileVfPvkjolGC1oIDkKe+pPu0u6uuDOqpT6vU61cGmoF4br/6DI4KPZWuzGEaBt839ThA7HmZ8mvQXonJileW5OUma6n+4SOIP7ga6kB0RrEmUhdIxtjT7k7tIcKJZZDCFKC3rNIZMU10= Received: from AM6PR0502MB3797.eurprd05.prod.outlook.com (52.133.21.26) by AM6PR0502MB3909.eurprd05.prod.outlook.com (52.133.21.158) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.386.5; Fri, 12 Jan 2018 07:24:05 +0000 Received: from AM6PR0502MB3797.eurprd05.prod.outlook.com ([fe80::b4b4:7de8:cf70:aa3a]) by AM6PR0502MB3797.eurprd05.prod.outlook.com ([fe80::b4b4:7de8:cf70:aa3a%13]) with mapi id 15.20.0386.008; Fri, 12 Jan 2018 07:24:05 +0000 From: Matan Azrad To: "Ananyev, Konstantin" , Thomas Monjalon , Gaetan Rivet , "Wu, Jingjing" CC: "dev@dpdk.org" , Neil Horman , "Richardson, Bruce" Thread-Topic: [PATCH v2 2/6] ethdev: add port ownership Thread-Index: AQHTihf/M9xg8LYorUSRFqZtTc27hqNtNdVQgAFomACAAAOCwIAAuwQAgABvY+A= Date: Fri, 12 Jan 2018 07:24:05 +0000 Message-ID: References: <1511870281-15282-1-git-send-email-matan@mellanox.com> <1515318351-4756-1-git-send-email-matan@mellanox.com> <1515318351-4756-3-git-send-email-matan@mellanox.com> <2601191342CEEE43887BDE71AB97725880E3B9D6@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772588627B12A@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772588627CCB0@irsmsx105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB9772588627CCB0@irsmsx105.ger.corp.intel.com> Accept-Language: en-US, he-IL Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=matan@mellanox.com; x-originating-ip: [85.64.136.190] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM6PR0502MB3909; 7:Ach/6VSgk/onDUBIk+9KKFoBZIPT3IhTkXhfPiXVcr+Ry8XH2NxAFFYu2FGKAwLjgcT59Dmo0P7EvIFYCAMgO5blLRmufubCK3+woAB9GY6J0n7Kd3/Xo7iM7aYXc6l3zKCa93ClKsSfkvLfRuDEYP4+7VBKZ08S4irwMetK80fbqEy2KsnQ9Ia+nZ52y7zsBpQ01lSWryOSKhIDCcRHNpIuFHpWkvzWKA3scP0hFr1C9OfLyo+PCSEmmOzjABjb x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: 1517e88f-ae68-4b34-2fd1-08d5598d76ab x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020078)(4652020)(5600026)(4604075)(3008032)(48565401081)(2017052603307)(7153060)(7193020); SRVR:AM6PR0502MB3909; x-ms-traffictypediagnostic: AM6PR0502MB3909: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(60795455431006)(180628864354917)(189930954265078)(45079756050767)(17755550239193); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040470)(2401047)(5005006)(8121501046)(93006095)(93001095)(3002001)(3231023)(944501141)(10201501046)(6055026)(6041268)(20161123558120)(20161123562045)(20161123564045)(20161123560045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(6072148)(201708071742011); SRVR:AM6PR0502MB3909; BCL:0; PCL:0; RULEID:(100000803101)(100110400095); SRVR:AM6PR0502MB3909; x-forefront-prvs: 0550778858 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(366004)(396003)(39380400002)(39860400002)(346002)(376002)(199004)(189003)(13464003)(33656002)(6116002)(6246003)(3846002)(99286004)(5250100002)(25786009)(53936002)(6306002)(55016002)(478600001)(14454004)(6436002)(45080400002)(9686003)(966005)(4326008)(81166006)(8676002)(93886005)(81156014)(54906003)(110136005)(316002)(5660300001)(2950100002)(8936002)(7736002)(305945005)(74316002)(2900100001)(68736007)(2906002)(105586002)(106356001)(575784001)(3660700001)(229853002)(76176011)(97736004)(59450400001)(86362001)(6506007)(66066001)(7696005)(102836004)(3280700002); DIR:OUT; SFP:1101; SCL:1; SRVR:AM6PR0502MB3909; H:AM6PR0502MB3797.eurprd05.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: ahpCwXRJw2DGs4p0gj0uxXbVqvaInizOe4b3q/DM2vWQYrS6KwcfIWrdAgslXXRiRBx1gP0zWAbMes2NqHxQZQ== spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1517e88f-ae68-4b34-2fd1-08d5598d76ab X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Jan 2018 07:24:05.8046 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR0502MB3909 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: Fri, 12 Jan 2018 07:24:08 -0000 Hi Konstantin From: Ananyev, Konstantin, Friday, January 12, 2018 2:02 AM > Hi Matan, >=20 > > > > 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, > > > > > Few comments from me below. > > > > > BTW, do you plan to add ownership mandatory check in control > > > > > path functions that change port configuration? > > > > > > > > No. > > > > > > So it still totally voluntary usage and application nneds to be > > > changed to exploit it? > > > Apart from RTE_FOR_EACH_DEV() change proposed by Gaetan? > > > > > > > Also RTE_FOR_EACH_DEV() change proposed by Gaetan is not protected > because 2 DPDK entities can get the same port while using it. >=20 > I am not talking about racing condition here. > Right now even from the same thread - I can call dev_configure() for the = port > which I don't own (let say it belongs to failsafe port), and that would r= emain, > correct? >=20 Yes. > > As I wrote in the log\docs and as discussed a lot in the first version: > > The new synchronization rules are: > > 1. The port allocation and port release synchronization will be > > managed by ethdev. > > 2. The port usage synchronization will be managed by the port owner. > > 3. The port ownership API synchronization(also with port creation) will= be > managed by ethdev. > > 4. DPDK entity which want to use a port must take ownership before. > > > > Ethdev should not protect 2 and 4 according these rules. > > > > > > > Konstantin > > > > > > > > > > > -----Original Message----- > > > > > > From: Matan Azrad [mailto:matan@mellanox.com] > > I mean the documentation about the needed alignment for spinlock. > Where is it? >=20 > https://emea01.safelinks.protection.outlook.com/?url=3Dhttp%3A%2F%2Finfo > center.arm.com%2Fhelp%2Findex.jsp%3Ftopic%3D%2Fcom.arm.doc.faqs%2 > Fka15414.html&data=3D02%7C01%7Cmatan%40mellanox.com%7Cb3c329ae9db > f4bd29a7008d5594fb776%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1 > %7C636513121294703050&sdata=3D40v3b4wk5f4qEyIY5jdDv8S47LjgXK0t9TPtav > XIMOk%3D&reserved=3D0 > https://emea01.safelinks.protection.outlook.com/?url=3Dhttp%3A%2F%2Finfo > center.arm.com%2Fhelp%2Findex.jsp%3Ftopic%3D%2Fcom.arm.doc.dht000 > 8a%2FCJAGCFAF.html&data=3D02%7C01%7Cmatan%40mellanox.com%7Cb3c32 > 9ae9dbf4bd29a7008d5594fb776%7Ca652971c7d2e4d9ba6a4d149256f461b%7 > C0%7C1%7C636513121294703050&sdata=3DB7pEZjFJntVp3Il8fS9wr%2FlxABgNX > FSr9PE4emEPLQE%3D&reserved=3D0 >=20 > Might be ARM and PPC guys can provide you some more complete/recent > docs. Thanks. =20 > > > > > 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. >=20 > Hmm, not sure why you can't do same checking with different lock or atomi= c > variable? >=20 The set ownership API is protected by ownership lock and checks the owner I= D validity=20 By reading the next owner ID. So, the owner ID allocation and set API should use the same atomic mechanis= m. The set(and others) ownership APIs already uses the ownership lock so I thi= nk it makes sense to use the same lock also in ID allocation. =20 > > > > > In fact, for next_owner_id, you don't need a lock - just > > > > > rte_atomic_t should be enough. > > > > > > > > I don't think so, it is problematic in next_owner_id wraparound > > > > and may > > > complicate the code in other places which read it. > > > > > > IMO it is not that complicated, something like that should work I thi= nk. > > > > > > /* init to 0 at startup*/ > > > rte_atomic32_t *owner_id; > > > > > > int new_owner_id(void) > > > { > > > int32_t x; > > > x =3D rte_atomic32_add_return(&owner_id, 1); > > > if (x > UINT16_MAX) { > > > rte_atomic32_dec(&owner_id); > > > return -EOVERWLOW; > > > } else > > > return x; > > > } > > > > > > > > > > Why not just to keep it simple and using the same lock? > > > > > > Lock is also fine, I just think it better be a separate one - that > > > would protext just next_owner_id. > > > Though if you are going to use uuid here - all that probably not > > > relevant any more. > > > > > > > I agree about the uuid but still think the same lock should be used for= both. >=20 > But with uuid you don't need next_owner_id at all, right? > So lock will only be used for rte_eth_dev_data[] fields anyway. > Sorry, I meant uint64_t, not uuid. > > > > > Another alternative would be to use 2 locks - one for > > > > > next_owner_id second for actual data[] protection. > > > > > > > > > > Another thing - you'll probably need to grab/release a lock > > > > > inside > > > > > rte_eth_dev_allocated() too. > > > > > It is a public function used by drivers, so need to be protected = too. > > > > > > > > > > > > > Yes, I thought about it, but decided not to use lock in next: > > > > rte_eth_dev_allocated > > > > rte_eth_dev_count > > > > rte_eth_dev_get_name_by_port > > > > rte_eth_dev_get_port_by_name > > > > maybe more... > > > > > > As I can see in patch #3 you protect by lock access to > > > rte_eth_dev_data[].name (which seems like a good thing). > > > So I think any other public function that access > > > rte_eth_dev_data[].name should be protected by the same lock. > > > > > > > I don't think so, I can understand to use the ownership lock here(as in= port > creation) but I don't think it is necessary too. > > What are we exactly protecting here? > > Don't you think it is just timing?(ask in the next moment and you may > > get another answer) I don't see optional crash. >=20 > Not sure what you mean here by timing... > As I understand rte_eth_dev_data[].name unique identifies device and is > used by port allocation/release/find functions. > As you stated above: > "1. The port allocation and port release synchronization will be managed= by > ethdev." > To me it means that ethdev layer has to make sure that all accesses to > rte_eth_dev_data[].name are atomic. > Otherwise what would prevent the situation when one process does > rte_eth_dev_allocate()->snprintf(rte_eth_dev_data[x].name, ...) while > second one does rte_eth_dev_allocated(rte_eth_dev_data[x].name, ...) ? >=20 The second will get True or False and that is it. Maybe if it had been called just a moment after, It might get different ans= wer.=20 Because these APIs don't change ethdev structure(just read), it can be OK. But again, I can understand to use ownership lock also here. > > > Static allocation is fine by me - I just said there is probably no > > > need to fail if description provide by use will be truncated in that = case. > > > Though if used description is *that* important - rte_malloc() can hel= p > here. > > > > > Again, what is the difference between port name and owner name > regarding the allocations? >=20 > As I understand rte_eth_dev_data[].name unique identifies device and > always has to be consistent. > owner.name is not critical for system operation, and I don't see a big de= al if it > would be truncated. >=20 > > The advantage of static allocation: > > 1. Not use protected malloc\free functions in other protected code. >=20 > You can call malloc/free before/after grabbing the lock. > But as I said - I am fine with static array here too - I just don't think= truncating > user description should cause a failure. >=20 Ok, will just add warning print in truncation case. > > 2. Easier to the user. > > > > > > > > > > > > + memset(port_owner->name, 0, > > > > > RTE_ETH_MAX_OWNER_NAME_LEN); > > > > > > + RTE_LOG(ERR, EAL, "Invalid owner name.\n"); > > > > > > + ret =3D -EINVAL; > > > > > > + goto unlock; > > > > > > + } > > > > > > + > > > > > > + port_owner->id =3D owner->id; > > > > > > + RTE_PMD_DEBUG_TRACE("Port %d owner is %s_%05d.\n", > port_id, > > > > > > + owner->name, owner->id); > > > > > > + > > > > > > > > > > As another nit - you can avoid all these gotos by restructuring c= ode a > bit: > > > > > > > > > > rte_eth_dev_owner_set(const uint16_t port_id, const struct > > > > > rte_eth_dev_owner *owner) { > > > > > rte_spinlock_lock(...); > > > > > ret =3D _eth_dev_owner_set_unlocked(port_id, owner); > > > > > rte_spinlock_unlock(...); > > > > > return ret; > > > > > } > > > > > > > > > Don't you like gotos? :) > > > > > > Not really :) > > > > > > > I personally use it only in error\performance scenarios. > > > > > > Same here - prefer to avoid them if possible. > > > > > > > Do you think it worth the effort? > > > > > > IMO - yes, well structured code is much easier to understand and > maintain. > > I don't think so in error cases(and performance), It is really clear he= re, but if > you are insisting, I will change it. > > Are you? >=20 > Yes, that would be my preference. > Why otherwise I would bother to write all this? :) >=20 > > (If the community thinks like you I think "goto" check should be added = to > checkpatch). >=20 > Might be there are pieces of code there goto are really hard to avoid, an= d/or > using goto would provide some performance benefit or so... > But that case definitely doesn't look like that. Let's stop "goto" discussion here, in spite of I don't think like you globa= lly, In this case I have no problem to change it.=20 Thanks, Matan.