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 6B6D41B206 for ; Sun, 5 Nov 2017 07:53:01 +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=dXVoc0BHGIg7Ws+z4p84QKrAKvZm/uBPJl3JwIFfWq8=; b=VeGCeYxl48lIWmL1q9R79T1wJPuAZ+ikHuxDA8f7HELy/CNJ9SEd2ZO6karELe3/6wAKwprzv+mHMovwO6j5XS/SHVEB5KEiTS+zuZuyMDZqGkxAmqGQI9WjWDlbkHeMaHIrJfFb2aRLoJru/VTo2hlO6NqiFwf61lOM5+aecas= Received: from HE1PR0502MB3659.eurprd05.prod.outlook.com (10.167.127.17) by HE1PR0502MB3657.eurprd05.prod.outlook.com (10.167.127.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.197.13; Sun, 5 Nov 2017 06:52:59 +0000 Received: from HE1PR0502MB3659.eurprd05.prod.outlook.com ([fe80::c524:908c:b99c:3f4b]) by HE1PR0502MB3659.eurprd05.prod.outlook.com ([fe80::c524:908c:b99c:3f4b%13]) with mapi id 15.20.0197.017; Sun, 5 Nov 2017 06:52:59 +0000 From: Matan Azrad To: Adrien Mazarguil CC: Gaetan Rivet , "dev@dpdk.org" Thread-Topic: [PATCH 2/3] net/mlx4: adjust removal error Thread-Index: AQHTVKSEVjpF2359YEyA2AF1sF6vyKMFR+zA Date: Sun, 5 Nov 2017 06:52:59 +0000 Message-ID: References: <1509637324-13525-1-git-send-email-matan@mellanox.com> <1509637324-13525-3-git-send-email-matan@mellanox.com> <20171103130555.GN24849@6wind.com> In-Reply-To: <20171103130555.GN24849@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; HE1PR0502MB3657; 6:zAXdMadnijmiwKWNpWu+ChpxIeRRTeiEq78QYfGvg1G4haTCbP+JLChdYxt0x+3NNNfmjHsUA0sk509Hf/Gc++AyU0+jxNrHnUpvYF4ugtYfmL98CSd967kIhf0YIVdiHbcYz+csMj8ixOxYO8l0XkxmFdn1cPp3HmoGXPa5qhEIFTrNEHbQ62uMfgyDbZFRRZHPtjFI/eGt8lp7fA40xQ5ZCJatOsuZrVSLf6xIF6hJXfeZa9a43C0ijJRRqf0rVKCgex1Ne+9r++z9ySyMBnrxMnVTq9ZzrDwtmDzaMQDz7WKRmlRADy/A7Xk8cLeru19jPaEfmTHHFGq+MUeaeVDOJkJY8kuI7Ts3kKQGplc=; 5:jH1gAK0Unwl7TIuud6J29kjaNA7XPcdyKZ0woHH96a0IlMhk8srHtMJPht6/GUgXVvToM06/dLPIUHJ1q8Z9uDlqOpF3lbPbQpQYosGHgAzMk3pSKVCrsI5/xvt21k/fKaB90A8/8HTXTsQuRV6Z8zLsC/EVGuJMz/FvVlywdzk=; 24:/5eXujcv39i2Ianbsg46uOBCXshbVZlL1St4d7K1B1jhbtr3O+WMJJ6nUMTQIfzrSaBy2aIU6yZD2tf0GLDyPx4/CGSG7wAh9TanoARxtIY=; 7:Qk5sp3WXfqPfAL3RDB0GspJ8Zr2Hh4ZvQr2MlngAuigYYWm7C5BrJe1scWLeTzruU1EZFqKbMN3ZHOWAyPGlyFFeEHmOs+a9n+XTtL0f61w+9fKLSrl+cqAH6Z3SVd/IZFnQY5KPO+epeDp9ReC4rkc9FCgrC1HGl1m6UxhK5rt6OZ7rO/2wHNIkE42jFmkCxNRmVQcUfWz1wBzte+NSafDA7UMSC4Q1Vv69apxEFfmo2RLVP/9TGLs1Mcf/5ufy x-ms-exchange-antispam-srfa-diagnostics: SSOS; x-ms-office365-filtering-correlation-id: 986a14c4-2afe-4229-5bc4-08d52419da60 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001)(48565401081)(4534020)(4602075)(4627115)(201703031133081)(201702281549075)(2017052603199); SRVR:HE1PR0502MB3657; x-ms-traffictypediagnostic: HE1PR0502MB3657: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-exchange-antispam-report-test: UriScan:(788757137089); x-microsoft-antispam-prvs: x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(8121501046)(5005006)(10201501046)(93006095)(93001095)(3002001)(100000703101)(100105400095)(3231021)(6055026)(6041248)(20161123555025)(20161123558100)(20161123560025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123562025)(20161123564025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:HE1PR0502MB3657; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:HE1PR0502MB3657; x-forefront-prvs: 04825EA361 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(346002)(376002)(189002)(13464003)(199003)(51914003)(24454002)(76104003)(7736002)(2900100001)(2906002)(106356001)(33656002)(478600001)(81166006)(101416001)(81156014)(68736007)(74316002)(14454004)(66066001)(105586002)(9686003)(4326008)(305945005)(55016002)(5660300001)(7696004)(53936002)(6916009)(2950100002)(8936002)(25786009)(8676002)(6506006)(6436002)(102836003)(6116002)(3846002)(6246003)(3280700002)(229853002)(5250100002)(50986999)(53546010)(189998001)(316002)(54356999)(3660700001)(97736004)(54906003)(76176999)(575784001)(86362001)(99286004); DIR:OUT; SFP:1101; SCL:1; SRVR:HE1PR0502MB3657; H:HE1PR0502MB3659.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) 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: 986a14c4-2afe-4229-5bc4-08d52419da60 X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Nov 2017 06:52:59.8542 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0502MB3657 Subject: Re: [dpdk-dev] [PATCH 2/3] net/mlx4: adjust removal error 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: Sun, 05 Nov 2017 06:53:01 -0000 Hi Adrien, Thanks for the review :) Please see below comments. > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Friday, November 3, 2017 3:06 PM > To: Matan Azrad > Cc: Gaetan Rivet ; dev@dpdk.org > Subject: Re: [PATCH 2/3] net/mlx4: adjust removal error >=20 > On Thu, Nov 02, 2017 at 03:42:03PM +0000, Matan Azrad wrote: > > Fail-safe PMD expects to get -ENODEV error value if sub PMD control > > command fails because of device removal. > > > > Make control callbacks return with -ENODEV when the device has > > disappeared. > > > > Signed-off-by: Matan Azrad >=20 > I think there are a several inconsistencies regarding the places where > mlx4_removed() is used, this could lead to mistakes or redundant calls to= this > function later on. >=20 > You have to choose between low-level internal functions (e.g. > mlx4_set_sysfs_ulong()) or user-facing ones from the eth_dev_ops > interface (e.g. mlx4_dev_set_link_up()), but neither intermediate functio= ns > nor a mix of all approaches. You are touching here, exactly in one of my design thoughts: Either using always "low" level error adjustments or using always high leve= l adjustments. The high level approach does less reuse of code but simpler to maintain (as= you said). I decided to combine the two approaches while never going to the lowest lev= el code(ibv, pipes). Adding the check in mlx4_dev_set_link() can replace two checks: in mlx4_dev= _set_link_up() and mlx4_dev_set_link_down().=20 Adding the check in mlx4_flow_toggle()can replace many checks: all flows ca= llbacks and also mlx4_mac_addr_add(),mlx4_mac_addr_set(). You right regarding mlx4_set_sysfs_ulong() it can be replaced by check in = mlx4_mtu_set() - will fix it in V2. You right regarding mlx4_ifreq(), it can be replaced by check in in mlx4_l= ink_update() - - will fix it in V2. I can understand the consistency approach but I think the above two cases t= o be in lower level functions are harmless and reuse code. What do you think? >=20 > Standardizing on low-level functions is not practical as it means you'd h= ave to > check for a device removal after each ibv_*() call. Therefore my suggesti= on is > to check it at the highest level, in all functions exposed though > mlx4_dev_ops in case of error, even innocuous one like > mlx4_stats_get() and those returning void (rte_errno can still be set), a= ll in > the name of consistency. >=20 If everything OK with the callback (even in a removal case) why to set rte_= errno? Specifically in mlx4_stats_get() has no error flow and we don't want error = return in case of removal since we can provide stats even after removal (SW= counters) and this is a good "feature" for failsafe plug out saving stats = process. =20 > The mlx4_removed() documentation should be updated to reflect the places > it's supposed to be called as well. All this means a larger patch is nece= ssary. >=20 Do you mean documentation in code(comment) or mlx4 docs, maybe both? > See below for coding style issues. >=20 > > --- > > drivers/net/mlx4/mlx4.h | 1 + > > drivers/net/mlx4/mlx4_ethdev.c | 38 > ++++++++++++++++++++++++++++++++++---- > > drivers/net/mlx4/mlx4_flow.c | 2 ++ > > drivers/net/mlx4/mlx4_intr.c | 5 ++++- > > drivers/net/mlx4/mlx4_rxq.c | 1 + > > drivers/net/mlx4/mlx4_txq.c | 1 + > > 6 files changed, 43 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/mlx4/mlx4.h b/drivers/net/mlx4/mlx4.h index > > e0a9853..cac9654 100644 > > --- a/drivers/net/mlx4/mlx4.h > > +++ b/drivers/net/mlx4/mlx4.h > > @@ -149,6 +149,7 @@ int mlx4_flow_ctrl_get(struct rte_eth_dev *dev, > > struct rte_eth_fc_conf *fc_conf); int > > mlx4_flow_ctrl_set(struct rte_eth_dev *dev, > > struct rte_eth_fc_conf *fc_conf); > > +int mlx4_removed(const struct priv *priv); > > > > /* mlx4_intr.c */ > > > > diff --git a/drivers/net/mlx4/mlx4_ethdev.c > > b/drivers/net/mlx4/mlx4_ethdev.c index b0acd12..76914b0 100644 > > --- a/drivers/net/mlx4/mlx4_ethdev.c > > +++ b/drivers/net/mlx4/mlx4_ethdev.c > > @@ -312,6 +312,8 @@ > > > > ret =3D mlx4_sysfs_write(priv, name, value_str, (sizeof(value_str) - = 1)); > > if (ret < 0) { > > + if (mlx4_removed(priv)) > > + ret =3D -ENODEV; > > DEBUG("cannot write %s `%s' (%lu) to sysfs: %s", > > name, value_str, value, strerror(rte_errno)); > > return ret; > > @@ -340,15 +342,19 @@ > > > > if (sock =3D=3D -1) { > > rte_errno =3D errno; > > - return -rte_errno; > > + goto error; > > } > > ret =3D mlx4_get_ifname(priv, &ifr->ifr_name); > > if (!ret && ioctl(sock, req, ifr) =3D=3D -1) { > > rte_errno =3D errno; > > - ret =3D -rte_errno; > > + close(sock); > > + goto error; > > } > > close(sock); > > return ret; > > +error: > > + mlx4_removed(priv); > > + return -rte_errno; > > } > > > > /** > > @@ -473,13 +479,17 @@ > > if (up) { > > err =3D mlx4_set_flags(priv, ~IFF_UP, IFF_UP); > > if (err) > > - return err; > > + goto error; > > } else { > > err =3D mlx4_set_flags(priv, ~IFF_UP, ~IFF_UP); > > if (err) > > - return err; > > + goto error; > > } > > return 0; > > +error: > > + if (mlx4_removed(priv)) > > + return -ENODEV; > > + return err; > > } > > > > /** > > @@ -947,6 +957,7 @@ enum rxmode_toggle { > > > > ifr.ifr_data =3D (void *)ðpause; > > if (mlx4_ifreq(priv, SIOCETHTOOL, &ifr)) { > > + mlx4_removed(priv); > > ret =3D rte_errno; > > WARN("ioctl(SIOCETHTOOL, ETHTOOL_GPAUSEPARAM)" > > " failed: %s", > > @@ -1002,6 +1013,7 @@ enum rxmode_toggle { > > else > > ethpause.tx_pause =3D 0; > > if (mlx4_ifreq(priv, SIOCETHTOOL, &ifr)) { > > + mlx4_removed(priv); > > ret =3D rte_errno; > > WARN("ioctl(SIOCETHTOOL, ETHTOOL_SPAUSEPARAM)" > > " failed: %s", > > @@ -1013,3 +1025,21 @@ enum rxmode_toggle { > > assert(ret >=3D 0); > > return -ret; > > } >=20 > Missing empty line. >=20 OK. > > +/** > > + * Check if mlx4 device was removed. >=20 > "mlx4" is a somewhat redundant given PMD name. >=20 > A separate paragraph should describe where this function is supposed to b= e > called. >=20 OK. > > + * > > + * @param priv > > + * Pointer to private structure. > > + * > > + * @return > > + * -ENODEV when device is removed and rte_errno is set, otherwise 0. > > + */ > > +int > > +mlx4_removed(const struct priv *priv) { > > + struct ibv_device_attr device_attr; > > + > > + if (ibv_query_device(priv->ctx, &device_attr) =3D=3D EIO) > > + return -(rte_errno =3D ENODEV); >=20 > Although a nice shortcut, coding rules don't allow this. You have to assi= gn > rte_errno on its own separate line. My suggestion if you want to avoid a > block would be to return 0 directly when !=3D EIO. >=20 Can you address me to this code rule documentation? > > + return 0; > > +} > > diff --git a/drivers/net/mlx4/mlx4_flow.c > > b/drivers/net/mlx4/mlx4_flow.c index 8b87b29..606c888 100644 > > --- a/drivers/net/mlx4/mlx4_flow.c > > +++ b/drivers/net/mlx4/mlx4_flow.c > > @@ -1069,6 +1069,8 @@ struct mlx4_drop { > > err =3D errno; > > msg =3D "flow rule rejected by device"; > > error: > > + if (mlx4_removed(priv)) > > + err =3D ENODEV; > > return rte_flow_error_set > > (error, err, RTE_FLOW_ERROR_TYPE_HANDLE, flow, msg); } > diff --git > > a/drivers/net/mlx4/mlx4_intr.c b/drivers/net/mlx4/mlx4_intr.c index > > b17d109..0ebdb28 100644 > > --- a/drivers/net/mlx4/mlx4_intr.c > > +++ b/drivers/net/mlx4/mlx4_intr.c > > @@ -359,7 +359,10 @@ > > ret =3D EINVAL; > > } > > if (ret) { > > - rte_errno =3D ret; > > + if (mlx4_removed(dev->data->dev_private)) > > + ret =3D ENODEV; > > + else > > + rte_errno =3D ret; > > WARN("unable to disable interrupt on rx queue %d", > > idx); > > } else { > > diff --git a/drivers/net/mlx4/mlx4_rxq.c b/drivers/net/mlx4/mlx4_rxq.c > > index 7fe21b6..43dad26 100644 > > --- a/drivers/net/mlx4/mlx4_rxq.c > > +++ b/drivers/net/mlx4/mlx4_rxq.c > > @@ -832,6 +832,7 @@ void mlx4_rss_detach(struct mlx4_rss *rss) > > ret =3D rte_errno; > > mlx4_rx_queue_release(rxq); > > rte_errno =3D ret; > > + mlx4_removed(priv); > > assert(rte_errno > 0); > > return -rte_errno; > > } > > diff --git a/drivers/net/mlx4/mlx4_txq.c b/drivers/net/mlx4/mlx4_txq.c > > index a9c5bd2..09bdfd8 100644 > > --- a/drivers/net/mlx4/mlx4_txq.c > > +++ b/drivers/net/mlx4/mlx4_txq.c > > @@ -372,6 +372,7 @@ struct txq_mp2mr_mbuf_check_data { > > ret =3D rte_errno; > > mlx4_tx_queue_release(txq); > > rte_errno =3D ret; > > + mlx4_removed(priv); > > assert(rte_errno > 0); > > return -rte_errno; > > } > > -- > > 1.8.3.1 > > >=20 > -- > Adrien Mazarguil > 6WIND