From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0073.outbound.protection.outlook.com [104.47.1.73]) by dpdk.org (Postfix) with ESMTP id D440B1B2F4 for ; Tue, 16 Jan 2018 18:20:29 +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=GpGZgD65xVHoGFhXJPSkGYMBiGGgc0JLJ0JIeEbxD4E=; b=olOI/bT1Glak7t4G5i04ncieC9aoxaisR9ZpPttdSdbOFu8vKHPOrtmdr3tSzu5reDwjzK1q85qfoEAXfuYCPFS4NyyaiX2/zxdYrzxgzPu+VJ4INQDJiH3y6bpp8GYY/0LHmauhM84uCUk1iyd/eaIkdj6DAitc9hDwpZbPZTI= Received: from AM6PR0502MB3797.eurprd05.prod.outlook.com (52.133.21.26) by AM6PR0502MB3864.eurprd05.prod.outlook.com (52.133.21.149) 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 17:20:27 +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 17:20:27 +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/VHV3LzKwTE6Nrr2gSXYAm6N2Z8DQgAAqzACAAAzw8IAAGE+AgAABgXA= Date: Tue, 16 Jan 2018 17:20:27 +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> <20180116165409.a463ukxbsmtdoc2v@bidouze.vm.6wind.com> In-Reply-To: <20180116165409.a463ukxbsmtdoc2v@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; AM6PR0502MB3864; 7:2uXSMdVv+z7zP0ryvBG5s/pCSH49k+aW0TFAxhlrOlrqwWsS0Jjs52B4JQTfhXJdKwRdjUGJMy8rv5scspGSWabgSeaCz5icMjZgPfksxs2NNJ9pDfgN4H/DNtWioILhRytUvwMtQ+Jfw8FnVHJBIGj1N64ePjssLrjrYVqDD3XUGZryuYG7HFS6s/DuiJJTd2VatpfR+n3WxI5fH/W87M9km0JoVo/11xT0zsvzQRn/STHH/AbVW5jGDuO/yI7L x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 25bb0f3a-83bd-4d19-e2ae-08d55d057002 x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(48565401081)(5600026)(4604075)(3008032)(2017052603307)(7153060)(7193020); SRVR:AM6PR0502MB3864; x-ms-traffictypediagnostic: AM6PR0502MB3864: 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)(93006095)(93001095)(3231023)(944501161)(10201501046)(6055026)(6041268)(20161123560045)(20161123562045)(20161123564045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123558120)(6072148)(201708071742011); SRVR:AM6PR0502MB3864; BCL:0; PCL:0; RULEID:(100000803101)(100110400095); SRVR:AM6PR0502MB3864; x-forefront-prvs: 0554B1F54F x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(376002)(346002)(366004)(396003)(39380400002)(39860400002)(199004)(189003)(24454002)(76104003)(6506007)(102836004)(97736004)(7696005)(3660700001)(5660300001)(478600001)(59450400001)(76176011)(74316002)(81156014)(8676002)(7736002)(14454004)(93886005)(8936002)(3280700002)(305945005)(99286004)(3846002)(316002)(54906003)(66066001)(81166006)(2906002)(6116002)(68736007)(26005)(9686003)(53936002)(6916009)(6436002)(55016002)(2900100001)(86362001)(4326008)(5250100002)(106356001)(6246003)(2950100002)(33656002)(105586002)(229853002)(25786009); DIR:OUT; SFP:1101; SCL:1; SRVR:AM6PR0502MB3864; 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: twN9U64WBJL12lx15cGxkcLynyoEAOhNUV/72LDj/otJ4E4QLIPVb68tkvs2ZOaJv+MaMVR4cmqXA9ed8PtVFw== 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: 25bb0f3a-83bd-4d19-e2ae-08d55d057002 X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Jan 2018 17:20:27.7312 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR0502MB3864 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 17:20:30 -0000 Hi Gaetan From: Ga=EBtan Rivet, Tuesday, January 16, 2018 6:54 PM > On Tue, Jan 16, 2018 at 04:15:36PM +0000, Matan Azrad wrote: > > 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 thi= s case. > > > > > > + > > > > > > - **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, f= ile). > > > > > Also, "get" as an action is not descriptive enough. > > > > > > > > > Isn't "get by device name" descriptive? > > > > > > The endgame is capturing a device that we know we are interested in. > > > The device name being used for matching is an implementation detail, > > > which should be abstracted by using a sub-function. > > > > > > Putting this in the name defeat the reason for using another function= . > > > > > > > > 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 > > > > using > > > 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? > > > > > > You are getting a port_id that is only valid for the rte_eth_devices > > > array, by using the ethdev iterator. You are only looking for an ethd= ev. > > > > > > 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. > > > > > > > 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. > > >=20 > Right :) . Call it fs_ethdev_portid_get() or fs_ethdev_find() then. >=20 Sure, agree with the first one. > > > 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. > > > > > > 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? > > > > > > > > > > You are touching at a pretty contentious subject here :) . > > > > > > 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= ... > > > > > > As it is, the device->name for PCI will match the name given as a > > > devargs, so functionally this should not change anything. > > > > > > 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. > > > > > > 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: > > > > > > # A physical port exists at 0000:00:02.0 > > > testpmd --vdev=3D"net_failsafe,dev(00:02.0)" -- -i > > > > > > Would fail to capture the device 0000:00:02.0, as this is the name > > > that the PCI bus would give to this device, in the absence of a user-= given > name. > > > > > > 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 a= n > rte_devargs). > > > Currently, this API does not exist. > > > > > > 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 get a binary device representation, and compare bytes per bytes > > > the binary representation given by your devargs and by the device- > >name. > > > > > > But this is a hack, and a pretty ugly one at that: you have no way > > > of knowing the size taken by this binary representation, so you can > > > restrict yourself to the vdev and PCI bus for the moment and take > > > the larger of an rte_vdev_driver pointer and an rte_pci_addr.... > > > > > > { > > > 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 */ > > > } > > > } > > > > > > So, really, really ugly. Anyway. > > > > > Yes, ugly :) Thanks for this update! > > Will keep the comparison by device->name. > > >=20 > Well as explained, above, the comparison by device->name only works with > whitelisted devices. >=20 > So either implement something broken right now that you will need to > update in 18.05, or implement it properly in 18.05 from the get go. >=20 For the current needs it is enough. We can also say that it is the user responsibility to pass to failsafe the = same names and same args as he passes for EAL(or default EAL names). I think I emphasized it in documentation. > > > > > > > > > > > > + /* 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 snprint= f) and > submit 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 new > > > devargs creation? > > > > Also to do the copy in parse level(same method as we are doing in > > > > probe > > > level)? > > > > > > > > > > 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_devi= ce). > > > > > > So keep everything here, it is the right place to deal with these thi= ngs. > > > > > 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 w= ork to > parse level and the probe work to probe level. >=20 > After re-reading, I think we misunderstood each other. > You cannot remove the rte_devargs created during parsing: it is allocated > alongside the sub_device structure. >=20 > You must only remove the rte_devargs allocated by the EAL (using > rte_eal_devargs_remove()). >=20 Sure. > Before removing it, you must copy its content in the local sub_device > rte_devargs structure. I only proposed a way to do this copy that would n= ot > deal with rte_devargs internals, as it is bound to evolve rather soon. > Yes. =20 > Otherwise, no, I do not want to complicate the parsing operations, they a= re > already too complicated and too criticals. Better to keep it all here. I think fs_parse_device function is not complicated and it is the natural p= lace for devargs games. For me this is the right place for the copy & remove devargs. Are you insisting to put all in fs_bus_init? > -- > Ga=EBtan Rivet > 6WIND