From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2F1C2A051F; Mon, 20 Jan 2020 16:41:14 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3732C1BC25; Mon, 20 Jan 2020 16:41:13 +0100 (CET) Received: from EUR04-VI1-obe.outbound.protection.outlook.com (mail-eopbgr80073.outbound.protection.outlook.com [40.107.8.73]) by dpdk.org (Postfix) with ESMTP id BE40E1B952 for ; Mon, 20 Jan 2020 16:41:11 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=eNFRosV8WU4nd+RBISOIBN/inPacSeImRWt/Y0p+Yfos+/ui0XZ0+0nzxQ3d4DDayygR4d1e3y9BvXBv7beel7I1aR1+HkBln6yUH4F2OxCsIv0cs5aB8oxxwlKQ7WA6yYPng1WSZwQqravUZ880VsXaGrXljcaUc7R0Xio4lZgMs809s3RHGsQ4IYhYnIU8ikiullcUakJY+TXDskpkWgQLTUL4mCEUx/6e6bV+1cimZhlcaAI7+r3juyq4PJ1A9rBhHgTgkvCrcrgsPCKa1mj1txHwtussgeln+7fDAS0f32EFiEYsaLnTZfHZDjrRQPIwDC0wRLXkyaHH/681vA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=0KVhzOMBQkx4kKxzMaNwax0tVWZOf/tkikSKKefqiXU=; b=cNhFTvYupM5zp5XeUvWppqzS5q7xTkrOSTpqAWswBtvH/ivcWEMw/gz1HWB6HJGOeD4ysHO6CuuvZMdzKuK+Q+T9K67RmA981orEoKrgFfzGeyYZXZQZBDQr2H6RS6GngN35oxa/zinlLih3SNrNZSZFTUaRyiGO4WmmRttDBt92UxtYoAQf28+KKJu7FLIKPpliwu5vSaU1fITW19o/ei51hzdjNFxwS7bKEgruNxZppGMtaRn2O1y5xk79ZlzXDrJtQJR9MNuT5tqpJhNNyBhlh/t1iwtVjvbnR+pRh44PqT66jpWhd01RGgIuhaq+GnUlHDsg6Dp2pKL5XWuduA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=mellanox.com; dmarc=pass action=none header.from=mellanox.com; dkim=pass header.d=mellanox.com; arc=none 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=0KVhzOMBQkx4kKxzMaNwax0tVWZOf/tkikSKKefqiXU=; b=I262dhm8jrB1Y6cLVlqY9956xLIjRznEvFQtii3mLKa2vkrhJgbUdRbQSnV37lrjQrHHU3nHg+7XtbFTTCpFOwI6h3DZp82EM5VHl3Qo4m5FUnGa+lnKdpvpnD64yG9yOCWbXuta7MzJLZgm/OYvN8taj4xRo8eLk5EwOGTOV98= Received: from AM4PR05MB3265.eurprd05.prod.outlook.com (10.171.188.154) by AM4PR05MB3361.eurprd05.prod.outlook.com (10.171.189.142) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2644.23; Mon, 20 Jan 2020 15:41:10 +0000 Received: from AM4PR05MB3265.eurprd05.prod.outlook.com ([fe80::68eb:ad79:71f4:110f]) by AM4PR05MB3265.eurprd05.prod.outlook.com ([fe80::68eb:ad79:71f4:110f%3]) with mapi id 15.20.2644.024; Mon, 20 Jan 2020 15:41:10 +0000 From: Slava Ovsiienko To: Olivier Matz CC: "dev@dpdk.org" , Matan Azrad , Raslan Darawsheh , Ori Kam , Shahaf Shuler , "stephen@networkplumber.org" , "thomas@mellanox.net" Thread-Topic: [PATCH v4 2/5] mbuf: detach mbuf with pinned external buffer Thread-Index: AQHVz5lfSYn4VmnN/ky7ozolTRV1nafzqjiA Date: Mon, 20 Jan 2020 15:41:10 +0000 Message-ID: References: <20191118094938.192850-1-shahafs@mellanox.com> <1579179869-32508-1-git-send-email-viacheslavo@mellanox.com> <1579179869-32508-3-git-send-email-viacheslavo@mellanox.com> <20200120135607.GI14387@glumotte.dev.6wind.com> In-Reply-To: <20200120135607.GI14387@glumotte.dev.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=viacheslavo@mellanox.com; x-originating-ip: [77.75.144.194] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 4d522b3f-d5ec-4dae-3e95-08d79dbf2c89 x-ms-traffictypediagnostic: AM4PR05MB3361:|AM4PR05MB3361: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtFwd x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 0288CD37D9 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(4636009)(396003)(376002)(366004)(39860400002)(136003)(346002)(189003)(199004)(81156014)(6506007)(2906002)(478600001)(52536014)(33656002)(66556008)(81166006)(66446008)(66946007)(5660300002)(8676002)(66476007)(64756008)(6916009)(76116006)(4326008)(86362001)(71200400001)(54906003)(316002)(26005)(7696005)(9686003)(186003)(8936002)(55016002)(290074003); DIR:OUT; SFP:1101; SCL:1; SRVR:AM4PR05MB3361; H:AM4PR05MB3265.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: LaGIjQBc+6cUnkbYMtkCa+zaxtTWh6GayFvcuNr+FhHheDAlL6iuP5EZhUIauvOG7icyuSR7IR7KQBNt/o1n/1/6IySXrA8KO8QVGMiNd+RzBMLhYGGxgWHTZb0GPc5wK3k60XDuprbBZ9oPLp+cKXUwzOh2HukaWJ3VoDfxN+x3XC1Y3EbUwinVWSccDHErAXkwRm1OmxJq77iFN0ygntjBD9jsGEqr+NMWsDi5M9m9HAJ9kCkVsij9EioFXjx11qA/AcpXmfocY7qcVRJfUdKUElxzJpsknJw027+Lvx2l/welnZ2UILYxC7cXIZWAEFhQod0R19ZEde0X/5jz4skBIDoP9sHrgNHj3Xh4XEYTwJyNojisTZgdZ5NUot3vNA31JBO0n6TXIHsQNTtRgRf75SjAaCDR/cxiZi7ZwiAcMoLruv2ixvobiiMU/55KzOm+ii0zLOv+sPZduGD5gCRTU25hLYfowaU1bYBFA+T4Wz++QELmY95y4ExmuXnT 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: 4d522b3f-d5ec-4dae-3e95-08d79dbf2c89 X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Jan 2020 15:41:10.6981 (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-CrossTenant-userprincipalname: jrtdyMfb4ZBq62oG2RI6FIver6Hi2A+jy2tkT3pVNMWB7ulNn6DMlWXhYpbASXPFFCOlvXlfbX0e3XnS95Jwv1yN9m+ojhLFij9RmvnJZd0= X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR05MB3361 Subject: Re: [dpdk-dev] [PATCH v4 2/5] mbuf: detach mbuf with pinned external buffer 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, Olivier Thanks a lot for the thorough review. There are some answers to comments, please, see below. > > > > /** > > + * @internal version of rte_pktmbuf_detach() to be used on mbuf freein= g. >=20 > -version > +Version >=20 > > + * For indirect and regular (not pinned) external mbufs the standard > > + * rte_pktmbuf is involved, for pinned external buffer mbufs the > > + special > > + * handling is performed: >=20 > Sorry, it is not very clear to me, especially what "the standard rte_pktm= buf is > involved" means. Sorry, it is mistype, should be read as "rte_pktmbuf_detach is invoked". >=20 > > + * > > + * - return zero if reference counter in shinfo is one. It means > > + there is > > + * no more references to this pinned buffer and mbuf can be returned > > + to >=20 > -references > +reference >=20 > > + * the pool > > + * > > + * - otherwise (if reference counter is not one), decrement > > +reference > > + * counter and return non-zero value to prevent freeing the backing m= buf. > > + * > > + * Returns non zero if mbuf should not be freed. > > + */ > > +static inline uint16_t __rte_pktmbuf_detach_on_free(struct rte_mbuf > > +*m) >=20 > I think int would be better than uint16_t >=20 > > +{ > > + if (RTE_MBUF_HAS_EXTBUF(m)) { > > + uint32_t flags =3D rte_pktmbuf_priv_flags(m->pool); > > + > > + if (flags & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF) { > > + struct rte_mbuf_ext_shared_info *shinfo; > > + > > + /* Clear flags, mbuf is being freed. */ > > + m->ol_flags =3D EXT_ATTACHED_MBUF; > > + shinfo =3D m->shinfo; > > + /* Optimize for performance - do not dec/reinit */ > > + if (likely(rte_mbuf_ext_refcnt_read(shinfo) =3D=3D 1)) > > + return 0; > > + /* > > + * Direct usage of add primitive to avoid > > + * duplication of comparing with one. > > + */ > > + if (likely(rte_atomic16_add_return > > + (&shinfo->refcnt_atomic, -1))) > > + return 1; > > + /* Reinitialize counter before mbuf freeing. */ > > + rte_mbuf_ext_refcnt_set(shinfo, 1); > > + return 0; > > + } > > + } > > + rte_pktmbuf_detach(m); > > + return 0; > > +} >=20 > I don't think the API comment really reflects what is done in this functi= on. In > my understanding, the detach() operation does nothing on an extmem > pinned mbuf. So detach() is probably not the proper name. >=20 > What about something like this instead: >=20 > /* [...]. > * assume m is pinned to external memory */ static inline int > __rte_pktmbuf_pinned_ext_buf_decref(struct rte_mbuf *m) { > struct rte_mbuf_ext_shared_info *shinfo; >=20 > /* Clear flags, mbuf is being freed. */ > m->ol_flags =3D EXT_ATTACHED_MBUF; > shinfo =3D m->shinfo; >=20 > /* Optimize for performance - do not dec/reinit */ > if (likely(rte_mbuf_ext_refcnt_read(shinfo) =3D=3D 1)) > return 0; >=20 > /* > * Direct usage of add primitive to avoid > * duplication of comparing with one. > */ > if (likely(rte_atomic16_add_return > (&shinfo->refcnt_atomic, -1))) > return 1; >=20 > /* Reinitialize counter before mbuf freeing. */ > rte_mbuf_ext_refcnt_set(shinfo, 1); > return 0; > } >=20 > static __rte_always_inline struct rte_mbuf * rte_pktmbuf_prefree_seg(stru= ct > rte_mbuf *m) { > __rte_mbuf_sanity_check(m, 0); >=20 > if (likely(rte_mbuf_refcnt_read(m) =3D=3D 1)) { >=20 > if (!RTE_MBUF_DIRECT(m)) > if (!RTE_MBUF_HAS_PINNED_EXTBUF(m)) > rte_pktmbuf_detach(m); > else if (__rte_pktmbuf_pinned_ext_buf_decref(m)) > return NULL; > } > ... > ... (and same below) ... >=20 >=20 > (just quickly tested) >=20 > The other advantage is that we don't call rte_pktmbuf_detach() where not > needed. Your proposal fetches the private flags for all indirect packets, including= the ones with IND_ATTACHED_MBUF flags (not external), this extra fetch and check mig= ht affect the performance for indirect packets (and it does not matter for packets wi= th external buffers). My approach updates the prefree routine for the packets with external buffers only, keeping intact the handling for all other mbuf types= .=20 >=20 > > + > > +/** > > * Decrease reference counter and unlink a mbuf segment > > * > > * This function does the same than a free, except that it does not > > @@ -1198,7 +1277,8 @@ static inline void rte_pktmbuf_detach(struct > rte_mbuf *m) > > if (likely(rte_mbuf_refcnt_read(m) =3D=3D 1)) { > > > > if (!RTE_MBUF_DIRECT(m)) > > - rte_pktmbuf_detach(m); > > + if (__rte_pktmbuf_detach_on_free(m)) > > + return NULL; > > > > if (m->next !=3D NULL) { > > m->next =3D NULL; > > @@ -1210,7 +1290,8 @@ static inline void rte_pktmbuf_detach(struct > rte_mbuf *m) > > } else if (__rte_mbuf_refcnt_update(m, -1) =3D=3D 0) { > > > > if (!RTE_MBUF_DIRECT(m)) > > - rte_pktmbuf_detach(m); > > + if (__rte_pktmbuf_detach_on_free(m)) > > + return NULL; > > > > if (m->next !=3D NULL) { > > m->next =3D NULL; > > -- > > 1.8.3.1 > >