From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <matan@mellanox.com>
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 <dev@dpdk.org>; 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 <matan@mellanox.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
CC: Gaetan Rivet <gaetan.rivet@6wind.com>, "dev@dpdk.org" <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: <HE1PR0502MB36595095A76859A523A9DF31D2530@HE1PR0502MB3659.eurprd05.prod.outlook.com>
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: <HE1PR0502MB3657C7711EB3DBC2FBD33517D2530@HE1PR0502MB3657.eurprd05.prod.outlook.com>
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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <matan@mellanox.com>
> Cc: Gaetan Rivet <gaetan.rivet@6wind.com>; 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 <matan@mellanox.com>
>=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