From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-eopbgr140058.outbound.protection.outlook.com [40.107.14.58]) by dpdk.org (Postfix) with ESMTP id 26D642C18 for ; Thu, 7 Mar 2019 12:34:44 +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=ms3sK8ezAnB5MNnLaXB69XLKdbsBiGwLjoRxjLQr1Ss=; b=g0nfhpF3At/RY6QhAOzADe4KVvloiCe7Wo/w6GJ3OMlsx3r0nYajKvKPQUOWcJyMonwwiYaeZL2uXrgWE40lsWPEULL+ARZ4RXhkR8o8IfPo0Z1STDSOWkV0x+JkAAYGLVIMJqnF2p8bScBvLvVArH5h5WRHXnuVV5vVapg6pL8= Received: from AM6PR05MB5926.eurprd05.prod.outlook.com (20.179.2.27) by AM6PR05MB6181.eurprd05.prod.outlook.com (20.178.94.222) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1643.15; Thu, 7 Mar 2019 11:34:43 +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 11:34:42 +0000 From: Raslan Darawsheh To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: Thomas Monjalon , "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: AQHU0zkXnxxf40CUoEe2KJZ0xt62sqX9TgaAgAAFfQCAARnDAIAAebiAgAD1mWCAABKmgIAAHT9A Date: Thu, 7 Mar 2019 11:34:42 +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> <20190307094757.56db3w74ftqkra7d@bidouze.vm.6wind.com> In-Reply-To: <20190307094757.56db3w74ftqkra7d@bidouze.vm.6wind.com> 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: dfff197b-c00b-41af-6810-08d6a2f0e48e 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:AM6PR05MB6181; x-ms-traffictypediagnostic: AM6PR05MB6181: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr x-microsoft-exchange-diagnostics: =?iso-8859-1?Q?1; AM6PR05MB6181; 23:QW5N+C8tXrQN3k/L1YX95YEXAa/ZUNJXYRvaH4O?= =?iso-8859-1?Q?aPDqM4astWSrlvdxzsftK5Vj4sY9OHyLHRkUf1U6UQyR5Y+xStMlDRHOF5?= =?iso-8859-1?Q?JNWnjgGVRGF439d0lG2BYhXmQT2wpI7J86D467Wr6qVozj0r8TC96oZ+0S?= =?iso-8859-1?Q?lU9Kq1tL9GZEpBfILgKrjwSFKsHR2rSaPoqNjJALt16hij6OThK++86Ojy?= =?iso-8859-1?Q?Sajr+6i9dCGVFJtGu9Qwy4sHqmdlXo49nS0YWVE7T8fHUv58G13rVv+uy3?= =?iso-8859-1?Q?UumFcTFcxrmcmD2Obsf5jvEe9X8hd1CapiAZ9xN5/hX7T4xMojdUJ1nke7?= =?iso-8859-1?Q?oqZDNuUTLCfahv0XOlFgvIoemof6H6mg7mKugELovy395HSPg9dYsolqW4?= =?iso-8859-1?Q?AVQd43sbE2fA+lc4DZvxEmj1yJmycX/ofYI8U1bSeEP3vSM9w+y7jBjgwo?= =?iso-8859-1?Q?muv8Fuk98v7YXwFvLzq7F1XD0+GEYmZJpafYcu+QnnJ/xzJFmJXCMIPVeu?= =?iso-8859-1?Q?7WnMdONn2OFSI2vr/Om971aY5AzuTUFQaX4PsUX8OeoyM2xDFnh8zMxNK3?= =?iso-8859-1?Q?AuoSi+l9bEE7ONEXq4dw7V7k0eqVoFXhXs3fdiqn5TGk3e7NVGIjFXNjYp?= =?iso-8859-1?Q?8g1hDhwuecZ/S05mukY1S1JNloyTYCtedm3DQWtNOwi1hZysfxm0uY9yMM?= =?iso-8859-1?Q?8x+Bv3rtGNk+pmEzmZMp2d9SK4doW2d47XpBO6xRAak8e9nYSfhjXLZYxt?= =?iso-8859-1?Q?FAaZVufDnnfePJ3oe0RTlMKAZWbJfsU1ys+XukepER/5viamnsuTnocR+9?= =?iso-8859-1?Q?45pqVsh0EEvbs2GXny80Tf9JRKkNlrnOAUysGPQzu71R6vkHUjFmmsBv19?= =?iso-8859-1?Q?L3+tpJiaj7g1tjIwcfLaNHxAzK48RSeoOa+c8QbFJRIqVFv9umNVxWAMGJ?= =?iso-8859-1?Q?NyTsigGbTmVsTaTDdfcqUgMk/VKmtCvDXHcQVE66c3Ir/ZJiQsgISQE09K?= =?iso-8859-1?Q?HCELcbws8pSQlkH8PCOSUCyXuHwSY+EVY/fqxhvy6GlZVu0KqcEUEvS2Tb?= =?iso-8859-1?Q?WxZqGIlAz1RWOBSe9l14Krm40YK2JThu8IA02/7nNnFXTno//TBDWJAWNG?= =?iso-8859-1?Q?rd/Td8mlkOgkDoNzyy+Wgmsi8ZMyfaaiMHihmVXXW5C1D3gf1sZ8kWqTmA?= =?iso-8859-1?Q?v0ZzOYPnv81Q+nFlK69fDoU+160iK38XsgPjbTjJ6zQUPxBBuvMwiA30r5?= =?iso-8859-1?Q?HWIok6TlXZu4dGBCxKF5PbVEygMWqXNE4sbqEaWmbOl2MN8hFPemIjKPUw?= =?iso-8859-1?Q?HNJP1pYe6gXC3VgaCsCFxaCRoCGX/nk1X7kOcFNOJOUZ3xxwkyixlI+BaY?= =?iso-8859-1?Q?EKLOZVcjVirrmSDQz6GBGuyVgp6BiXfNogllpjLkfvBBwoMQ+CA=3D=3D?= x-microsoft-antispam-prvs: x-forefront-prvs: 096943F07A x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(39860400002)(366004)(376002)(346002)(396003)(136003)(199004)(189003)(13464003)(6246003)(229853002)(71190400001)(5024004)(5660300002)(81156014)(76176011)(55016002)(53936002)(93886005)(97736004)(8676002)(7736002)(305945005)(8936002)(14454004)(486006)(74316002)(7696005)(9686003)(14444005)(68736007)(102836004)(81166006)(33656002)(316002)(186003)(99286004)(86362001)(11346002)(53546011)(3846002)(446003)(561944003)(6116002)(478600001)(54906003)(256004)(66574012)(66066001)(2906002)(106356001)(6436002)(26005)(52536013)(476003)(6506007)(4326008)(105586002)(71200400001)(25786009)(6916009); DIR:OUT; SFP:1101; SCL:1; SRVR:AM6PR05MB6181; 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: dzQCZClg97Lmfe05JSheUkuTEHlkdoyswwiM2ohqzcxfEU7PQiNCfGiq92Qnw0CvuqkdHx7q+IPtKKlXsIB1b+09gIguVQPuJRpWOzpSvhFMdqYxapeIZT1+oF6eHxEtd8ZdjrT0btTOQ+GvaAUyherhmDZ2yBJBNUk+doBPtXiQF/RwPh6Qr2pAFBVulIghM+cqPOFaDa/RHZBkooD1hDSIqUnHrBbW4fTH5bospNNgb8E4qkOlNj456ShR6NQf859eMBmMJwJ4OocHePFd75M9PiFKceHxkgPJJG2BbUBLNKWAThp8GipqZ704KkT86rMxvqan/OYyl+KHLKgQ1shuxhDvrn998E3Xp6H7Ry+2Vn2F4rlR3LV6BekwwYHHFJ1L72+mLR1NN1N/M4ZnVDwPw8ZA0M68nggvBNmN/Iw= 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: dfff197b-c00b-41af-6810-08d6a2f0e48e X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Mar 2019 11:34:42.8231 (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: AM6PR05MB6181 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 11:34:44 -0000 Hi, > -----Original Message----- > From: Ga=EBtan Rivet > Sent: Thursday, March 7, 2019 11:48 AM > To: Raslan Darawsheh > Cc: Thomas Monjalon ; 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 > On Thu, Mar 07, 2019 at 08:43:01AM +0000, Raslan Darawsheh wrote: > > 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-device with shared data > > > > > > 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? > > > > > > Yes indeed, there is no guard. > > > That's something not clear in DPDK, previously we were attaching > > > some vdevs in secondary only. > > > > > > > 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? > > > > > > 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 and not removing the EAL device. > > > This is a generic question about deciding whether we want all ethdev > > > ports to be synced in multi-process or not. > > > > > > In failsafe context, we are closing the EAL device and change the > > > state of the sub-device accordingly. So I think there is no issue. > > > > > > > > > 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_d= ev > 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 | | PR= IMARY > > > > | | | +dev_priv-+ | | | > > > > | | dev_private +-->| | | | | > > > > | | ... | | | | | | > > > > | | port_id | | | | | | > > > > | | | | | | | | > > > > | | | | | | | | > > > > | | | | | +----------------------+ | > > > > | | | | | +- rte_eth_devices[n] -+ | > > > > | | | | | | | | SE= CONDARY > > > > | | | | | | | | > > > > | | | | | | | | > > > > | | | | | | | | > > > > | | | +---------+ | | | > > > > | | |<---------------+ data | | > > > > | +------------------+ +----------------------+ | > > > > +--------------------------------------------------------------+ > > > > > > > > Here port_id is used within fail-safe to get back to rte_eth_device= s[n]. > > > > This disappears once a device is closed, as all shared space is zer= oed. > > > > > > > > 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 clo= sed. > > > > This seems dangerous. > > > > > > The state of the sub-device is changed. > > > I don't see any issue. > > > > > > > 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 c= orrect > accross all processes. > > > > > > > > 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(). > > > > > > 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. > > > > > > I have only one doubt: look at the macro in this patch: > > > > > > #define ETH(sdev) \ > > > ((sdev)->data =3D=3D NULL ? NULL : &rte_eth_devices[(sdev)->data- > > > >port_id]) > > > > > > 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 > plugged out devices during TX. >=20 > Ok, thanks for your insights Thomas and Raslan. Sorry about the rambling > above I needed to write down the stuff to think about it. >=20 > You can use RTE_MAX_ETHPORTS as a sentinel value for port_id, this way th= e > value is kept unsigned and there are several checks against this specific= value > otherwise. >=20 > so ETH(sdev) could be >=20 > (PORT_ID(sdev) >=3D RTE_MAX_ETHPORTS ? NULL : > &rte_eth_devices[PORT_ID(sdev)]) >=20 But, this mean that you have to explicitly set the port id to RTE_MAX_ETH_= PORTS in the sdev after fs_dev_remove and you shouldn't rely on the port ID= anymore. > -- > Ga=EBtan Rivet > 6WIND Kindest regards Raslan Darawsheh