From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0079.outbound.protection.outlook.com [104.47.2.79]) by dpdk.org (Postfix) with ESMTP id 56EC11B2F3 for ; Tue, 16 Jan 2018 17:15:38 +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=fgp05xU9h0BdP8G13wuNzGld0pFnyYIAw+vuP6KyzlI=; b=iLwUO7b2WrLWMBfSZO/9NuB2an5u7zK//HX0bmOyyF1NnWVTn1RyKry28aBog03/R0GzvMDvYH9uQ+f0Wm4ghTdHe36n1m064OItvl0LbMqbscvvskpaJpTm7buDVbR2XlbNjGyWC+7X+z0JQADi5LBMGZgwFAl7aOTX9tG5jeU= Received: from AM6PR0502MB3797.eurprd05.prod.outlook.com (52.133.21.26) by AM6PR0502MB4069.eurprd05.prod.outlook.com (52.133.30.156) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.407.7; Tue, 16 Jan 2018 16:15:36 +0000 Received: from AM6PR0502MB3797.eurprd05.prod.outlook.com ([fe80::6c28:c6b3:de94:a733]) by AM6PR0502MB3797.eurprd05.prod.outlook.com ([fe80::6c28:c6b3:de94:a733%13]) with mapi id 15.20.0407.012; Tue, 16 Jan 2018 16:15:36 +0000 From: Matan Azrad To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: Ferruh Yigit , Thomas Monjalon , "dev@dpdk.org" , "stephen@networkplumber.org" Thread-Topic: [PATCH v3 3/8] net/failsafe: support probed sub-devices getting Thread-Index: AQHTjrp/VHV3LzKwTE6Nrr2gSXYAm6N2Z8DQgAAqzACAAAzw8A== Date: Tue, 16 Jan 2018 16:15:36 +0000 Message-ID: References: <20171222173846.20731-1-adrien.mazarguil@6wind.com> <1515509253-17834-1-git-send-email-matan@mellanox.com> <1515509253-17834-4-git-send-email-matan@mellanox.com> <20180116110920.vqp3bqjroudsdjm4@bidouze.vm.6wind.com> <20180116144050.ho4k2dp24lgzhtdr@bidouze.vm.6wind.com> In-Reply-To: <20180116144050.ho4k2dp24lgzhtdr@bidouze.vm.6wind.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: [193.47.165.251] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM6PR0502MB4069; 7:mH/QgOxmMtNPc/fTAODkE++kWfmah+yggV2nBRKrz/ZV9OZeLV7V2tBaaZyIO2KAucOht7WEh4DgTZQA0oEsauZzdOrxL+uYXYXIxYCdkf75jUib9oTBmwR5z8XkBi6zU8OZr2qYel9nbTCtR57XrULrUyLwmAVd6J6N5/C1jVwTJbsd+uwXkR4soMpvHqqXf9+tagytbttWHslLtRrXnUs0idfGvKnHp2NSD9K3MJOXep+55bbzyVVdtRsoFy0h x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: d92848d7-fa31-48e9-bca4-08d55cfc608e x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(5600026)(4604075)(3008032)(48565401081)(2017052603307)(7153060)(7193020); SRVR:AM6PR0502MB4069; x-ms-traffictypediagnostic: AM6PR0502MB4069: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040470)(2401047)(8121501046)(5005006)(3002001)(10201501046)(93006095)(93001095)(3231023)(944501161)(6055026)(6041268)(20161123564045)(20161123560045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(20161123558120)(6072148)(201708071742011); SRVR:AM6PR0502MB4069; BCL:0; PCL:0; RULEID:(100000803101)(100110400095); SRVR:AM6PR0502MB4069; x-forefront-prvs: 0554B1F54F x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(39860400002)(366004)(39380400002)(376002)(346002)(396003)(189003)(199004)(76104003)(24454002)(5660300001)(9686003)(6506007)(54906003)(53936002)(4326008)(6436002)(97736004)(229853002)(86362001)(59450400001)(2900100001)(102836004)(2950100002)(6916009)(55016002)(2906002)(99286004)(33656002)(93886005)(68736007)(3660700001)(106356001)(6246003)(81156014)(14454004)(5250100002)(8676002)(105586002)(3280700002)(76176011)(7696005)(25786009)(66066001)(316002)(3846002)(7736002)(305945005)(26005)(6116002)(74316002)(478600001)(8936002)(81166006); DIR:OUT; SFP:1101; SCL:1; SRVR:AM6PR0502MB4069; H:AM6PR0502MB3797.eurprd05.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: Ekb8NWQWa2AHUdFxA+gvo25FQC1k8/hrS8Yw1ohPjB4u0XOMY6U0kb5UGHWR4MPVlykXRCfEQe3R6gxDiw2Tew== spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: d92848d7-fa31-48e9-bca4-08d55cfc608e X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Jan 2018 16:15:36.3180 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR0502MB4069 Subject: Re: [dpdk-dev] [PATCH v3 3/8] net/failsafe: support probed sub-devices getting 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, 16 Jan 2018 16:15:38 -0000 Hi Gaetan From: Ga=EBtan Rivet, Tuesday, January 16, 2018 4:41 PM > On Tue, Jan 16, 2018 at 12:27:57PM +0000, Matan Azrad wrote: > > Hi Gaetan > > > > From: Ga=EBtan Rivet, Tuesday, January 16, 2018 1:09 PM > > > Hi Matan, > > > > > > I'n not fond of the commit title, how about: > > > > > > [PATCH v3 3/8] net/failsafe: add probed etherdev capture > > > > > > ? > > > > > OK, no problem. > > > > > On Tue, Jan 09, 2018 at 02:47:28PM +0000, Matan Azrad wrote: > > > > Previous fail-safe code didn't support getting probed sub-devices > > > > and failed when it tried to probe them. > > > > > > > > Skip fail-safe sub-device probing when it already was probed. > > > > > > > > Signed-off-by: Matan Azrad > > > > Cc: Gaetan Rivet > > > > --- > > > > doc/guides/nics/fail_safe.rst | 5 ++++ > > > > drivers/net/failsafe/failsafe_eal.c | 60 > > > > ++++++++++++++++++++++++------------- > > > > 2 files changed, 45 insertions(+), 20 deletions(-) > > > > > > > > diff --git a/doc/guides/nics/fail_safe.rst > > > > b/doc/guides/nics/fail_safe.rst index 5b1b47e..b89e53b 100644 > > > > --- a/doc/guides/nics/fail_safe.rst > > > > +++ b/doc/guides/nics/fail_safe.rst > > > > @@ -115,6 +115,11 @@ Fail-safe command line parameters > > > > order to take only the last line into account (unlike ``exec()``= ) at every > > > > probe attempt. > > > > > > > > +.. note:: > > > > + > > > > + In case of whitelist sub-device probed by EAL, fail-safe PMD > > > > + will take the > > > device > > > > + as is, which means that EAL device options are taken in this ca= se. > > > > + > > > > - **mac** parameter [MAC address] > > > > > > > > This parameter allows the user to set a default MAC address to > > > > the fail-safe diff --git a/drivers/net/failsafe/failsafe_eal.c > > > > b/drivers/net/failsafe/failsafe_eal.c > > > > index 19d26f5..7bc7453 100644 > > > > --- a/drivers/net/failsafe/failsafe_eal.c > > > > +++ b/drivers/net/failsafe/failsafe_eal.c > > > > @@ -36,39 +36,59 @@ > > > > #include "failsafe_private.h" > > > > > > > > static int > > > > +fs_get_port_by_device_name(const char *name, uint16_t *port_id) > > > > > > The naming convention for the failsafe driver is > > > > > > namespace_object_sub-object_action() > > > > > OK. > > > With an ordering of objects by their scope (std, rte, failsafe, file)= . > > > Also, "get" as an action is not descriptive enough. > > > > > Isn't "get by device name" descriptive? >=20 > The endgame is capturing a device that we know we are interested in. > The device name being used for matching is an implementation detail, whic= h > should be abstracted by using a sub-function. >=20 > Putting this in the name defeat the reason for using another function. >=20 > > > static int > > > fs_ethdev_capture(const char *name, uint16_t *port_id); > > > > > You miss here the main reason why we need this function instead of usin= g > rte_eth_dev_get_port_by_name. > > The reason we need this function is because we want to find the device = by > the device name and not ethdev name. > > What's about fs_port_capture_by_device_name? >=20 > You are getting a port_id that is only valid for the rte_eth_devices arra= y, by > using the ethdev iterator. You are only looking for an ethdev. >=20 > So it doesn't really matter whether you are using the ethdev name or the > device name, in the end you are capturing an ethdev > --> fs_ethdev_capture seems good for me. >=20 I don't think so, this function doesn't take(capture) the device, just gets= its ethdev port id using the device name. The function which actually captures the device is the fs_bus_init. So maybe even the "capture" name looks problematic here. The main idea of this function is just to get the port_id. > Now, I guess you will say that the user would need to know that they have= to > provide a device name that would be written in device->name. The issue > here is that you have a leaky abstraction for your function, forcing this= kind of > consideration on your function user. >=20 > So I'd go further and will ask you to change the `const char *name` to a = `const > rte_devargs *da` in the parameters. > > > Maybe comparing it to device->devargs->name is better, What do you > think? > > >=20 > You are touching at a pretty contentious subject here :) . >=20 > Identifying devices is not currently a well-defined function in DPDK. > Some ports (actually, only one model: ConnectX-3) will have several ports > using the same PCI slot. But even ignoring this glaring problem... >=20 > As it is, the device->name for PCI will match the name given as a devargs= , so > functionally this should not change anything. >=20 > Furthermore, you will have devices probed without any devargs. The fail- > safe would thus be unable to capture non-blacklisted devices when the PCI > bus is in blacklist mode. >=20 > These not-blacklisted devices actually will have a full-PCI name (DomBDF > format), so a simple match with the one passed in your fail-safe devargs = will > fail, ex: >=20 > # A physical port exists at 0000:00:02.0 > testpmd --vdev=3D"net_failsafe,dev(00:02.0)" -- -i >=20 > Would fail to capture the device 0000:00:02.0, as this is the name that t= he PCI > bus would give to this device, in the absence of a user-given name. >=20 > In 18.05, or 18.08 there should be an EAL function that would be able to > identify a device given a specific ID string (very close to an rte_devarg= s). > Currently, this API does not exist. >=20 > You can hack your way around this for the moment, IF you really, really > want: parse your devargs, get the bus, use the bus->parse() function to g= et a > binary device representation, and compare bytes per bytes the binary > representation given by your devargs and by the device->name. >=20 > But this is a hack, and a pretty ugly one at that: you have no way of kno= wing > the size taken by this binary representation, so you can restrict yoursel= f to > the vdev and PCI bus for the moment and take the larger of an > rte_vdev_driver pointer and an rte_pci_addr.... >=20 > { > union { > rte_vdev_driver *drv; > struct rte_pci_addr pci_addr; > } bindev1, bindev2; > memset(&bindev1, 0, sizeof(bindev1)); > memset(&bindev2, 0, sizeof(bindev2)); > rte_eal_devargs_parse(device->name, da1); > rte_eal_devargs_parse(your_devstr, da2); > RTE_ASSERT(da1->bus =3D=3D rte_bus_find_by_name("pci") || > da1->bus =3D=3D rte_bus_find_by_name("vdev")); > RTE_ASSERT(da2->bus =3D=3D rte_bus_find_by_name("pci") || > da2->bus =3D=3D rte_bus_find_by_name("vdev")); > da1->bus->parse(da1->name, &bindev1); > da1->bus->parse(da2->name, &bindev2); > if (memcmp(&bindev1, &bindev2, sizeof(bindev1)) =3D=3D 0) { > /* found the device */ > } else { > /* not found */ > } > } >=20 > So, really, really ugly. Anyway. >=20 Yes, ugly :) Thanks for this update! Will keep the comparison by device->name. > >=20 > > > > + /* Take control of device probed by EAL options. */ > > > > + DEBUG("Taking control of a probed sub device" > > > > + " %d named %s", i, da->name); > > > > > > In this case, the devargs of the probed device must be copied within > > > the sub- device definition and removed from the EAL using the proper > > > rte_devargs API. > > > > > > Note that there is no rte_devargs copy function. You can use > > > rte_devargs_parse instead, "parsing" again the original devargs into > > > the sub- device one. It is necessary for complying with internal > > > rte_devargs requirements (da->args being malloc-ed, at the moment, > but may evolve). > > > > > > The rte_eal_devargs_parse function is not easy enough to use right > > > now, you will have to build a devargs string (using snprintf) and sub= mit it. > > > I proposed a change this release for it but it will not make it for > > > 18.02, that would have simplified your implementation. > > > > > > > Got you. You right we need to remove the created devargs in fail-safe > parse level. > > What do you think about checking it in the parse level and avoid the ne= w > devargs creation? > > Also to do the copy in parse level(same method as we are doing in probe > level)? > > >=20 > Not sure I follow here, but the new rte_devargs is part of the sub-device= (it is > not a pointer, but allocated alongside the sub_device). >=20 > So keep everything here, it is the right place to deal with these things. > But it will prevent the double parsing and also saves the method: If the device already parsed - copy its devargs and continue. If the device already probed - copy the device pointer and continue. I think this is the right dealing, no? Why to deal with parse level in probe level? Just keep all the parse work = to parse level and the probe work to probe level. Thanks, Matan. =20 > -- > Ga=EBtan Rivet > 6WIND