From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id CAB5E1B017 for ; Tue, 23 Jan 2018 14:34:52 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Jan 2018 05:34:51 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,401,1511856000"; d="scan'208";a="13501467" Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by fmsmga002.fm.intel.com with ESMTP; 23 Jan 2018 05:34:34 -0800 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by IRSMSX103.ger.corp.intel.com (163.33.3.157) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 23 Jan 2018 13:34:34 +0000 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.236]) by irsmsx112.ger.corp.intel.com ([169.254.1.12]) with mapi id 14.03.0319.002; Tue, 23 Jan 2018 13:34:33 +0000 From: "Ananyev, Konstantin" To: Matan Azrad , =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: Thomas Monjalon , "Wu, Jingjing" , "dev@dpdk.org" , Neil Horman , "Richardson, Bruce" Thread-Topic: [PATCH v3 7/7] app/testpmd: adjust ethdev port ownership Thread-Index: AQHTkHpyuUKMeuPQw06Oud7boKI8uKN7InaggAAFDoCAAAQmUIAACAgAgAAXyICABIDDwIAAGvuAgAB1A3CAANJcgIAAP6aQ Date: Tue, 23 Jan 2018 13:34:32 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725886282587@irsmsx105.ger.corp.intel.com> References: <1515318351-4756-1-git-send-email-matan@mellanox.com> <1516293317-30748-1-git-send-email-matan@mellanox.com> <1516293317-30748-8-git-send-email-matan@mellanox.com> <2601191342CEEE43887BDE71AB97725886280A68@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725886280AE8@irsmsx105.ger.corp.intel.com> <20180119150017.mljpcdmldqx32mkq@bidouze.vm.6wind.com> <2601191342CEEE43887BDE71AB97725886281B1D@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725886281E73@irsmsx105.ger.corp.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMGZlNDZkZDMtNmFjYS00ZTUzLThiNDYtYzk4NzE2OWYyYjRhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IkxtQStGSTJpcTJvWWpZS1Y3WmxFZkw0enltbW1URUdKY1ljN1FDYXFPdk09In0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 7/7] app/testpmd: adjust ethdev 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: Tue, 23 Jan 2018 13:34:53 -0000 Hi Matan, >=20 >=20 > Hi Konstantin > From: Ananyev, Konstantin, Monday, January 22, 2018 10:49 PM > > Hi Matan, > > > > > -----Original Message----- > > > From: Matan Azrad [mailto:matan@mellanox.com] > > > Sent: Monday, January 22, 2018 1:23 PM > > > To: Ananyev, Konstantin ; Ga=EBtan Rive= t > > > > > > Cc: Thomas Monjalon ; Wu, Jingjing > > > ; dev@dpdk.org; Neil Horman > > > ; Richardson, Bruce > > > > > > Subject: RE: [PATCH v3 7/7] app/testpmd: adjust ethdev port ownership > > > > > > > > > Hi > > > From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com] > > > > Hi lads, > > > > > > > > > > > > > > Hi Matan, > > > > > > > > > > On Fri, Jan 19, 2018 at 01:35:10PM +0000, Matan Azrad wrote: > > > > > > Hi Konstantin > > > > > > > > > > > > From: Ananyev, Konstantin, Friday, January 19, 2018 3:09 PM > > > > > > > > -----Original Message----- > > > > > > > > From: Matan Azrad [mailto:matan@mellanox.com] > > > > > > > > Sent: Friday, January 19, 2018 12:52 PM > > > > > > > > To: Ananyev, Konstantin ; > > > > > > > > Thomas Monjalon ; Gaetan Rivet > > > > > > > ; > > > > > > > > Wu, Jingjing > > > > > > > > Cc: dev@dpdk.org; Neil Horman ; > > > > > > > > Richardson, Bruce > > > > > > > > Subject: RE: [PATCH v3 7/7] app/testpmd: adjust ethdev port > > > > > > > > ownership > > > > > > > > > > > > > > > > Hi Konstantin > > > > > > > > > > > > > > > > From: Ananyev, Konstantin, Friday, January 19, 2018 2:38 PM > > > > > > > > > To: Matan Azrad ; Thomas Monjalon > > > > > > > > > ; Gaetan Rivet > > > > ; > > > > > > > Wu, > > > > > > > > > Jingjing > > > > > > > > > Cc: dev@dpdk.org; Neil Horman ; > > > > > > > > > Richardson, Bruce > > > > > > > > > Subject: RE: [PATCH v3 7/7] app/testpmd: adjust ethdev > > > > > > > > > port ownership > > > > > > > > > > > > > > > > > > Hi Matan, > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > > From: Matan Azrad [mailto:matan@mellanox.com] > > > > > > > > > > Sent: Thursday, January 18, 2018 4:35 PM > > > > > > > > > > To: Thomas Monjalon ; Gaetan Rivet > > > > > > > > > > ; Wu, Jingjing > > > > > > > > > > > > > > > > > > > > Cc: dev@dpdk.org; Neil Horman ; > > > > > > > Richardson, > > > > > > > > > > Bruce ; Ananyev, Konstantin > > > > > > > > > > > > > > > > > > > > Subject: [PATCH v3 7/7] app/testpmd: adjust ethdev port > > > > > > > > > > ownership > > > > > > > > > > > > > > > > > > > > Testpmd should not use ethdev ports which are managed b= y > > > > > > > > > > other DPDK entities. > > > > > > > > > > > > > > > > > > > > Set Testpmd ownership to each port which is not used by > > > > > > > > > > other entity and prevent any usage of ethdev ports whic= h > > > > > > > > > > are not owned by > > > > > > > Testpmd. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Matan Azrad > > > > > > > > > > --- > > > > > > > > > > app/test-pmd/cmdline.c | 89 +++++++++++++++++++--= ---- > > ---- > > > > ------- > > > > > > > ---- > > > > > > > > > ----- > > > > > > > > > > app/test-pmd/cmdline_flow.c | 2 +- > > > > > > > > > > app/test-pmd/config.c | 37 ++++++++++--------- > > > > > > > > > > app/test-pmd/parameters.c | 4 +- > > > > > > > > > > app/test-pmd/testpmd.c | 63 ++++++++++++++++++++-= --- > > ---- > > > > ---- > > > > > > > > > > app/test-pmd/testpmd.h | 3 ++ > > > > > > > > > > 6 files changed, 103 insertions(+), 95 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/app/test-pmd/cmdline.c > > > > > > > > > > b/app/test-pmd/cmdline.c index > > > > > > > > > > 31919ba..6199c64 100644 > > > > > > > > > > --- a/app/test-pmd/cmdline.c > > > > > > > > > > +++ b/app/test-pmd/cmdline.c > > > > > > > > > > @@ -1394,7 +1394,7 @@ struct cmd_config_speed_all { > > > > > > > > > > &link_speed) < 0) > > > > > > > > > > return; > > > > > > > > > > > > > > > > > > > > - RTE_ETH_FOREACH_DEV(pid) { > > > > > > > > > > + RTE_ETH_FOREACH_DEV_OWNED_BY(pid, > > my_owner.id) { > > > > > > > > > > > > > > > > > > Why do we need all these changes? > > > > > > > > > As I understand you changed definition of > > > > > > > > > RTE_ETH_FOREACH_DEV(), so no testpmd should work ok > > > > > > > > > default > > > > (no_owner case). > > > > > > > > > Am I missing something here? > > > > > > > > > > > > > > > > Now, After Gaetan suggestion RTE_ETH_FOREACH_DEV(pid) will > > > > > > > > iterate > > > > > > > over all valid and ownerless ports. > > > > > > > > > > > > > > Yes. > > > > > > > > > > > > > > > Here Testpmd wants to iterate over its owned ports. > > > > > > > > > > > > > > Why? Why it can't just iterate over all valid and ownerless p= orts? > > > > > > > As I understand it would be enough to fix current problems an= d > > > > > > > would allow us to avoid any changes in testmpd (which I think > > > > > > > is a good > > > > thing). > > > > > > > > > > > > Yes, I understand that this big change is very daunted, But I > > > > > > think the current a lot of bugs in testpmd(regarding port > > > > > > ownership) even more > > > > > daunted. > > > > > > > > > > > > Look, > > > > > > Testpmd initiates some of its internal databases depends on > > > > > > specific port iteration, In some time someone may take ownershi= p > > > > > > of Testpmd > > > > ports and testpmd will continue to touch them. > > > > > > > > But if someone will take the ownership (assign new owner_id) that > > > > port will not appear in RTE_ETH_FOREACH_DEV() any more. > > > > > > > > > > Yes, but testpmd sometimes depends on previous iteration using intern= al > > database. > > > So it uses internal database that was updated by old iteration. > > > > That sounds like just a bug in testpmd that need to be fixed, no? >=20 > If Testpmd already took ownership for these ports(like I did), it is ok. >=20 Hmm, why not just to fix testpmd, if there is a bug? As I said all control ops here are done by one thread, so it should be pret= ty easy. Or are you talking about race conditions? > > Any particular places where outdated device info is used? >=20 > For example, look for the stream management in testpmd(I think I saw it t= here). Anything particular? >=20 > > > > > If I look back on the fail-safe, its sole purpose is to have > > > > > seamless hotplug with existing applications. > > > > > > > > > > Port ownership is a genericization of some functions introduced b= y > > > > > the fail-safe, that could structure DPDK further. It should allow > > > > > applications to have a seamless integration with subsystems using > > > > > port ownership. Without this, port ownership cannot be used. > > > > > > > > > > Testpmd should be fixed, but follow the most common design > > > > > patterns of DPDK applications. Going with port ownership seems > > > > > like a paradigm shift. > > > > > > > > > > > In addition > > > > > > Using the old iterator in some places in testpmd will cause a > > > > > > race for run- > > > > time new ports(can be created by failsafe or any hotplug code): > > > > > > - testpmd finds an ownerless port(just now created) by the old > > > > > > iterator and start traffic there, > > > > > > - failsafe takes ownership of this new port and start traffic t= here. > > > > > > Problem! > > > > > > > > Could you shed a bit more light here - it would be race condition > > > > between whom and whom? > > > > > > Sure. > > > > > > > As I remember in testpmd all control ops are done within one thread > > > > (main lcore). > > > > > > But other dpdk entity can use another thread, for example: > > > Failsafe uses the host thread(using alarm callback) to create a new p= ort and > > to take ownership of a port. > > > > Hm, and you create new ports inside failsafe PMD, right and then set ne= w > > owner_id for it? >=20 > Yes. >=20 > > And all this in alarm in interrupt thread? >=20 > Yes. >=20 > > If so I wonder how you can guarantee that no-one else will set differen= t > > owner_id between > > rte_eth_dev_allocate() and rte_eth_dev_owner_set()? >=20 > I check it (see failsafe patch to this series - V5). > Function: fs_bus_init. You are talking about that peace of code: + ret =3D rte_eth_dev_owner_set(pid, &PRIV(dev)->my_owner); + if (ret) { + INFO("sub_device %d owner set failed (%s)," + " will try again later", i, strerror(ret)); + continue; right? So you just wouldn't include that device into your failsafe device. But that probably not what user wanted, especially if he bothered to create a special new low-level device for you. If that' s the use case, then I think you need to set device ownership at c= reation time - inside dev_allocate(). Again that would avoid such racing conditions inside testpmd. >=20 > > Could you point me to that place (I am not really familiar with familia= r with > > failsafe code)? > > > > > > > > The race: > > > Testpmd iterates over all ports by the master thread. > > > Failsafe takes ownership of a port by the host thread and start using= it. > > > =3D> The two dpdk entities may use the device at same time! > > > > Ok, if failsafe really assigns its owner_id(s) to ports that are alread= y in use by > > the app, then how such scheme supposed to work at all? >=20 > If the app works well (with the new rules) it already took ownership and = failsafe will see it and will wait until the application release it. Ok, and why application would need to release it? How it would know that failsafe device wants to use it now? Again where is a guarantee that after app released it some other entity wou= ldn't grab it for itself? > Every dpdk entity should know which port it wants to manage, > If 2 entities want to manage the same device - it can be ok and port own= ership can synchronize the usage. >=20 > Probably, application which will run fail-safe wants to manage only the f= ail-safe port and therefor to take ownership only for it. >=20 > > I.E. application has a port - it assigns some owner_id !=3D 0 to it, th= en PMD tries > > to set its owner_id tot the same port. > > Obviously failsafe's set_owner() will always fail in such case. > > > Yes, and will try again after some time. Same question again - how app will know that it has to release the port own= ership? >=20 > > From what I hear we need to introduce a concept of 'default owner id'. > > I.E. when failsafe PMD is created - user assigns some owner_id to it (d= efault). > > Then failsafe PMD generates it's own owner_id and assigns it only to th= e > > ports whose current owner_id is equal either 0 or 'default' owner_id. > > >=20 > It is a suggestion and we need to think about it more (I'm talking about = it with Gaetan in another thread). > Actually I think, if we want a generic solution to the generic problem th= e current solution is ok. >>From what I heard - every app that wants to use failsafe PMD would require = quite a lot of changes. It doesn't look ok to me. >=20 > > > > > > Obeying the new ownership rules can prevent all these races. > > > > > > > When we discussed RFC of owner_id patch, I thought we all agreed that > > owner_id API shouldn't be mandatory - i.e. existing apps not required = to > > change to work normally with that. >=20 > Yes, it is not mandatory if app doesn't use hotplug. >=20 > I think with hotplug it is mandatory in the most cases. Yes in failsafe you always install this alarm handler, so even if the app would have its own way to handle hotplug devices - it would suddenly need to use this new owner API - even if it doesn't need = to. Why it has to be? >=20 > And it can ease the secondary process model too. >=20 > Again, in the generic ownership problem as discussed in RFC: > Every entity, include app, should know which ports it wants to manage and= to take ownership only for them. >=20 > > Though right now it seems that application changes seems necessary, at = least > > to work ok with failsafe PMD. >=20 > And for solving the generic problem of ownership.(will defend from future= issues by sure). >=20 > > Which makes we wonder was it some sort of misunderstanding or we did we > > do something wrong here? >=20 > Mistakes can be done all the time, but I think we are all understand the = big issue of ownership and how the current solution solves it. > fail-safe it is just a current example for the problems which are possibl= e because of the generic ownership issue. Honestly that seems too much changes for the app just to make failsafe PMD = work correctly. IMO - It should be some way to support it without causing changes in each D= PDK application - otherwise something is wrong with the PMD itself. If let say that ownership model is required to make failsafe PMD to operate= - it should be done in a transparent way to the user. Probably something like Gaetan suggested in another mail or so. Konstantin >=20 > Thanks, > Matan > > Konstantin > > > > > > The only way to attach/detach port with it - invoke testpmd CLI > > > > "attach/detach" port. > > > > > > > > Konstantin > > > > > > > > > > > > > > Testpmd does not handle detection of new port. If it did, testing > > > > > fail-safe with it would be wrong. > > > > > > > > > > At startup, RTE_ETH_FOREACH_DEV already fixed the issue of > > > > > registering DEFERRED ports. There are still remaining issues > > > > > regarding this, but I think they should be fixed. The architectur= e > > > > > does not need to be completely moved to port ownership. > > > > > > > > > > If anything, this should serve as a test for your API with common > > > > > applications. I think you'd prefer to know and debug with testpmd > > > > > instead of firing up VPP or something like that to determine what > > > > > went wrong with using the fail-safe. > > > > > > > > > > > > > > > > > In addition > > > > > > As a good example for well-done application (free from ownershi= p > > > > > > bugs) I tried here to adjust Tespmd to the new rules and BTW to > > > > > > fix a > > > > > lot of bugs. > > > > > > > > > > Testpmd has too much cruft, it won't ever be a good example of a > > > > > well-done application. > > > > > > > > > > If you want to demonstrate ownership, I think you should start an > > > > > example application demonstrating race conditions and their mitig= ation. > > > > > > > > > > I think that would be interesting for many DPDK users. > > > > > > > > > > > > > > > > > > > > > > > So actually applications which are not aware to the port > > > > > > ownership still are exposed to races, but if there use the old > > > > > > iterator(with the new > > > > > change) the amount of races decreases. > > > > > > > > > > > > Thanks, Matan. > > > > > > > Konstantin > > > > > > > > > > > > > > > > > > > > > > > I added to Testpmd ability to take an ownership of ports as > > > > > > > > the new ownership and synchronization rules suggested, Sinc= e > > > > > > > > Tespmd is a DPDK entity which wants that no one will touch > > > > > > > > its owned ports, It must allocate > > > > > > > an unique ID, set owner for its ports (see in main function) > > > > > > > and recognizes them by its owner ID. > > > > > > > > > > > > > > > > > Konstantin > > > > > > > > > > > > > > > > > > > Regards, > > > > > -- > > > > > Ga=EBtan Rivet > > > > > 6WIND