From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR03-VE1-obe.outbound.protection.outlook.com (mail-eopbgr50089.outbound.protection.outlook.com [40.107.5.89]) by dpdk.org (Postfix) with ESMTP id 36BA32C17 for ; Thu, 7 Mar 2019 09:43:03 +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:X-MS-Exchange-SenderADCheck; bh=dRTm9z1sF/8ycEffdV/SUF9gSqbCeBVdlEU7mvbNXPk=; b=jpu67IobCKtOWqAmYDj3KD4ee7ARDCSCG2fPHVSg0Z3kg3PZ4VD98cgGpPwM3UolGWu6mmZ9ZP5Kyx6cW+O3JfS1dLndqRpEPs6/TQOR9W5oV0Wqo5WJVPoQ8+ig0UzS6VvR6xr52a9Jnk1RzYEzjbbw6IKP2tVAVxlTgZ7AaxY= Received: from AM6PR05MB5926.eurprd05.prod.outlook.com (20.179.2.27) by AM6PR05MB4968.eurprd05.prod.outlook.com (20.177.36.17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1686.18; Thu, 7 Mar 2019 08:43:01 +0000 Received: from AM6PR05MB5926.eurprd05.prod.outlook.com ([fe80::a0b3:8140:c1f1:dd89]) by AM6PR05MB5926.eurprd05.prod.outlook.com ([fe80::a0b3:8140:c1f1:dd89%3]) with mapi id 15.20.1686.018; Thu, 7 Mar 2019 08:43:01 +0000 From: Raslan Darawsheh To: Thomas Monjalon , =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: "dev@dpdk.org" , "stephen@networkplumber.org" Thread-Topic: [dpdk-dev] [PATCH v2 3/4] net/failsafe: replace local sub-device with shared data Thread-Index: AQHU0zkXnxxf40CUoEe2KJZ0xt62sqX9TgaAgAAFfQCAARnDAIAAebiAgAD1mWA= Date: Thu, 7 Mar 2019 08:43:01 +0000 Message-ID: References: <1551779507-10857-1-git-send-email-rasland@mellanox.com> <172443910.dRk5910QrU@xps> <20190306104632.fia2r2sz5yajvkne@bidouze.vm.6wind.com> <6541291.VHOuPfexjb@xps> In-Reply-To: <6541291.VHOuPfexjb@xps> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=rasland@mellanox.com; x-originating-ip: [82.213.2.186] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 866dfaba-c6ad-45fd-e59b-08d6a2d8e88e x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390118)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600127)(711020)(4605104)(4618075)(2017052603328)(7153060)(7193020); SRVR:AM6PR05MB4968; x-ms-traffictypediagnostic: AM6PR05MB4968: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-exchange-diagnostics: =?iso-8859-1?Q?1; AM6PR05MB4968; 23:VDcz9QF/cNhyQ/d2Kszrif4gGjh43vjfSGWd+CT?= =?iso-8859-1?Q?NgwBFqyVm+4kYT8QG5VydD2EDUZMLK8BE9Txv77cHfeF2l30oE1+lzBYZD?= =?iso-8859-1?Q?iMd+DVUwWoFW1xxHYEYamCguW+xON3DQVlfJoum6SIIUPGqvWPrlMfn3Su?= =?iso-8859-1?Q?uPcPfx5IzdwJ5v8qG/RdlVpY5gHUim2giWsCZmfRt0nzr1BsQag5hlQzDD?= =?iso-8859-1?Q?vyL782X9KK1LrbG+er9Ze9mztHIZfRbL1GpbAqSKOJ45Fr07bXWx4Tjoyd?= =?iso-8859-1?Q?6h5nkVeY/O5uZ5ZGML7er35YHFdvKM9l1CRx/dy1KDklZ0D1nrNTNQk8QV?= =?iso-8859-1?Q?Ngal759mTDCjw3+LKGAjuylqAqkKKkz5Ibk4qiIIR6tTkWcQ2x2Ql2B41n?= =?iso-8859-1?Q?sQy0mxL7113YXsVKMeUNQtt9YDTBgLpb+Mb9Q37OQQi8AwiPjlphIsbvVu?= =?iso-8859-1?Q?j4kwaeEN4mkhncizqXQfKkbjmtvcHZlv+qC9F6C1W0FDNTnMNc0mEuu1fe?= =?iso-8859-1?Q?taApldBwINIvF+finDNywfmv2SC9Ln+41j8pQ0g+YtSyuzTNeEtCk3uqzi?= =?iso-8859-1?Q?KCoeuAJ6xu9d9XErU0cB8bTUrF1ipMRITI/oq6FkG6dnSh8Owgj2fZm81c?= =?iso-8859-1?Q?sg1Gdhb+rid3bW+5sHiz6nOj9Js3JP6csi8U1ug3E0u0nzqhQCHCoR0PMg?= =?iso-8859-1?Q?NzKVRvBmaomcIuaXPZwV7yPnFduY8i7W9Mv66RTNyfjRw9H8c0O2rTUUWO?= =?iso-8859-1?Q?VATFZ3+PjMlU2SW0E0Bnu/Jwt3drXDnSZ0DpKa+W7DGPX3wSfFj3iByMJp?= =?iso-8859-1?Q?Ba4BPDBNZuy5NTv4ar7nwskuYhJciJjMLuc7GTAy2WQLgbjKKjl/mIY/PK?= =?iso-8859-1?Q?aPl5cwXlabNOU94I7CLfO4h/K0BomqNyvFqzhBOHGryWXj3uQgqYdn/936?= =?iso-8859-1?Q?3tG7ywZ2NJ7fPIC6a2wb33KRpx34AK1ICkv+DjkiIrNyBu/9dcZr5A03/s?= =?iso-8859-1?Q?tgn+yBOgrQ0jnPfnXifkFvl3D06Butc0A6t3e44ItKuBZhxlJnQFnbEw1K?= =?iso-8859-1?Q?sC/eA/kmlD0pUYGx05Cl4In4+hh7P50aavPWvF4b5UVx/C4XNwPCerDwA/?= =?iso-8859-1?Q?8lKgCl7d4PMpCRoZ0pLedzlMfrecyrxwwPr6sByz9GKNJ+0zxCTJ0uZdVL?= =?iso-8859-1?Q?J8HLG3sGDo+dmmGN7v0K6YhTeDS2SpWHTFrm4O8JiXSl53LhHK38+MA5nQ?= =?iso-8859-1?Q?Xjk4lg/oZDwCPQRsTTIZZtQAQDJ/zjDWZUiQjxpUxbhmKP11BGYt5X9WdJ?= =?iso-8859-1?Q?tvc1cHPq/Mrr7SlmSA5KDLqR93P/Q97KBRSAOhV5lj3ki6fFA/3fxafmbC?= =?iso-8859-1?Q?qnjwn6pjWxLqaRyR6sT4KSifw2dsM1e6bVmWhamjPgGy4NXUpQ8+1H4yzu?= =?iso-8859-1?Q?KQ/d4ukfhJreoGcmfuVipmJ3C0pWA597z/hrbrugDgen08vuQaDbtlo4uO?= =?iso-8859-1?Q?CzfsHofi0OF9KiCnS3pw9nEIGI371eVZB6WIsZ6oB?= x-microsoft-antispam-prvs: x-forefront-prvs: 096943F07A x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(979002)(396003)(136003)(366004)(39860400002)(376002)(346002)(199004)(189003)(13464003)(14444005)(5024004)(256004)(11346002)(71190400001)(446003)(66574012)(486006)(105586002)(106356001)(71200400001)(14454004)(476003)(68736007)(25786009)(4326008)(81156014)(8676002)(8936002)(66066001)(81166006)(26005)(3846002)(6116002)(6436002)(99286004)(53546011)(6506007)(76176011)(102836004)(5660300002)(74316002)(97736004)(7736002)(6246003)(305945005)(478600001)(229853002)(7696005)(86362001)(33656002)(93886005)(186003)(110136005)(52536013)(9686003)(316002)(53936002)(2906002)(54906003)(55016002)(561944003)(969003)(989001)(999001)(1009001)(1019001); DIR:OUT; SFP:1101; SCL:1; SRVR:AM6PR05MB4968; H:AM6PR05MB5926.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam-message-info: VFLgchTs+0ZBDC02406ghcJsTz7gEif3b2LE/CWFyqPOwO89zSypX1JQGY+mP/yfrzqT/so7js6IijeO4fDIiZPxoiT/OVPD9dfZ9QMMd9raQ3HzqCN0GHh2CGse9djSY2nnKymGIoWhGZooPOlluEpc9rkFQtEyg4KDs6ez6zHvw6RsHgkS6rqdzgGUyOqK2/xJdGSIUC4a1JvJHzInLI8wlzRinWjFqlZgG65SaIF75r7EpaVgBGaf0IULAhHm7NbAx9WjL3raOjQBirlE2L1Ye9Ch8XxlAywQyQ88JCozlxbPGbwGpkxhf3lO4El2SJchhrnxeCbHD/C+w1WX2l1CCKsIE+0ZMaKBawfVHNHDuqDJr8KxVq2G1q4/Z3kknwLaLN9wjW4M9uiKw1o2kjHOzOkhYFB7GAD+qouEKmc= 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: 866dfaba-c6ad-45fd-e59b-08d6a2d8e88e X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Mar 2019 08:43:01.6917 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR05MB4968 Subject: Re: [dpdk-dev] [PATCH v2 3/4] net/failsafe: replace local sub-device with shared data 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: Thu, 07 Mar 2019 08:43:03 -0000 Hi, > -----Original Message----- > From: Thomas Monjalon > Sent: Wednesday, March 6, 2019 8:02 PM > To: Ga=EBtan Rivet ; Raslan Darawsheh > > Cc: dev@dpdk.org; stephen@networkplumber.org > Subject: Re: [dpdk-dev] [PATCH v2 3/4] net/failsafe: replace local sub-de= vice > with shared data >=20 > 06/03/2019 11:46, Ga=EBtan Rivet: > > On Tue, Mar 05, 2019 at 06:58:04PM +0100, Thomas Monjalon wrote: > > > 05/03/2019 18:38, Ga=EBtan Rivet: > > > > What happens when a primary process closes a device before a > secondary? > > > > Is the secondary unable to stop / close its own then? Isn't there > > > > some missing uninit? > > > > > > Is the secondary process supposed to do any closing? > > > The device management should be done only by the primary process. > > > > > > Note: anyway all this hotplug related code should be dropped from > > > failsafe to be replaced by EAL hotplug management. > > > > > > > I don't know, I've never used secondary process. > > However, cursory reading the code of rte_eth_dev_close(), I don't see > > a guard against calling it from a secondary process? >=20 > Yes indeed, there is no guard. > That's something not clear in DPDK, previously we were attaching some > vdevs in secondary only. >=20 > > Reading code like > > > > rte_free(dev->data->rx_queues); > > dev->data->rx_queues =3D NULL; > > > > within makes me think the issue has been seen at least once, where > > shared data is freed multiple times, so I guessed some secondary > > processes were calling it. Maybe they are not meant to, but what > > prevents them from being badly written? > > > > Also, given rte_dev_remove IPC call to transfer the order to the > > primary, it seems that at least secondary processes are expected to > > call > > rte_dev_remove() at some point? So are they only authorized to call > > rte_dev_remove() (to manage hotplug), but not rte_eth_dev_close()? Is > > there a specific documentation detailing the design of secondary > > process and the related responsibilities in the lifetime of a device? > > How are they synching their rte_eth_devices list if they are not > > calling rte_eth_dev_close(), ever? >=20 > All these calls should be done in primary. > The IPC mechanism calls the attach/detach in secondary at EAL level. > The PMDs does the bridge between EAL device and ethdev port status. > But you are right, there can be a sync issue if closing an ethdev port an= d not > removing the EAL device. > This is a generic question about deciding whether we want all ethdev port= s to > be synced in multi-process or not. >=20 > In failsafe context, we are closing the EAL device and change the state o= f the > sub-device accordingly. So I think there is no issue. >=20 > > > > This seems dangerous to me. Why not instead allocating a > > > > per-process slab of memory that would hold the relevant references > > > > and outlive the shared data (a per-process rte_eth_dev private data= ...). > > > > > > Which data do you think should be allocated per process? > > > > > > > > > > [-------- SHARED SPACE --------------] [-- PER-PROCESS --------] > > +--------------------------------------------------------------+ > > | +------------------+ +- rte_eth_devices[n] -+ | > > | |rte_eth_dev_data |<---------------+ data | | PRIMAR= Y > > | | | +dev_priv-+ | | | > > | | dev_private +-->| | | | | > > | | ... | | | | | | > > | | port_id | | | | | | > > | | | | | | | | > > | | | | | | | | > > | | | | | +----------------------+ | > > | | | | | +- rte_eth_devices[n] -+ | > > | | | | | | | | SECOND= ARY > > | | | | | | | | > > | | | | | | | | > > | | | | | | | | > > | | | +---------+ | | | > > | | |<---------------+ data | | > > | +------------------+ +----------------------+ | > > +--------------------------------------------------------------+ > > > > Here port_id is used within fail-safe to get back to rte_eth_devices[n]= . > > This disappears once a device is closed, as all shared space is zeroed. > > > > This means that sometimes ETH(sdev) and PORT_ID(sdev) is correct, and > > at some point it is not anymore, once a sub-device has been closed. > > This seems dangerous. >=20 > The state of the sub-device is changed. > I don't see any issue. >=20 > > I was thinking initially that allocating a place where each sdev would > > store their rte_eth_devices / port_id back-reference could alleviate > > the issue, meaning that the fail-safe would not zero it on > > sdev_close(), and it would remain valid for the lifetime of a > > sub-device, so even when a sub-device is in DEV_PROBED state. > > > > But now that I think about it, it could probably be simpler: instead > > of using (ETH(sdev)->data->port_id) for the port_id of an sdev > > (meaning that it is dependent on the lifetime of the sdev, instead of > > the lifetime of the failsafe), the port-id itself should be stored in > > the sub_device structure. This structure will be available for the > > lifetime of the failsafe, and the port_id is correct accross all proces= ses. > > > > So PORT_ID(sdev) would be defined to something like (sdev->port_id), > > and > > ETH(sdev) would be (&rte_eth_devices[PORT_ID(sdev)]). It would remain > > correct even once the primary has closed the sub-device. > > > > What do you think? Do you agree that the current state is dangerous, > > and do you think the solution would alleviate the issue? Maybe the > > concern is unfounded and the only issue is within fs_dev_remove(). >=20 > Yes it is only seen in fs_dev_remove(). > I discussed about your proposal with Raslan, and we agree we could change > from sub_device.data to sub_device.port_id, it may be more future-proof. >=20 > I have only one doubt: look at the macro in this patch: >=20 > #define ETH(sdev) \ > ((sdev)->data =3D=3D NULL ? NULL : &rte_eth_devices[(sdev)->data- > >port_id]) >=20 > The NULL check cannot be done with a port id. > I think it was needed to manage one case. Raslan? That's right since we need it for fs_tx_unsafe, to add a protection for plu= gged out devices during TX. >=20 >=20 Kindest regards Raslan Darawsheh