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 9CB44A051F; Mon, 20 Jan 2020 17:17:32 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CC5EC1BE90; Mon, 20 Jan 2020 17:17:31 +0100 (CET) Received: from proxy.6wind.com (host.76.145.23.62.rev.coltfrance.com [62.23.145.76]) by dpdk.org (Postfix) with ESMTP id 0496B1BC25 for ; Mon, 20 Jan 2020 17:17:29 +0100 (CET) Received: from glumotte.dev.6wind.com (unknown [10.16.0.195]) by proxy.6wind.com (Postfix) with ESMTP id CC48A36FC0D; Mon, 20 Jan 2020 17:17:29 +0100 (CET) Date: Mon, 20 Jan 2020 17:17:29 +0100 From: Olivier Matz To: Slava Ovsiienko Cc: "dev@dpdk.org" , Matan Azrad , Raslan Darawsheh , Ori Kam , Shahaf Shuler , "stephen@networkplumber.org" , "thomas@mellanox.net" Message-ID: <20200120161729.GM14387@glumotte.dev.6wind.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) 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, On Mon, Jan 20, 2020 at 03:41:10PM +0000, Slava Ovsiienko wrote: > 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 freeing. > > > > -version > > +Version > > > > > + * For indirect and regular (not pinned) external mbufs the standard > > > + * rte_pktmbuf is involved, for pinned external buffer mbufs the > > > + special > > > + * handling is performed: > > > > Sorry, it is not very clear to me, especially what "the standard rte_pktmbuf is > > involved" means. > > Sorry, it is mistype, should be read as "rte_pktmbuf_detach is invoked". > > > > > + * > > > + * - 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 > > > > -references > > +reference > > > > > + * the pool > > > + * > > > + * - otherwise (if reference counter is not one), decrement > > > +reference > > > + * counter and return non-zero value to prevent freeing the backing mbuf. > > > + * > > > + * Returns non zero if mbuf should not be freed. > > > + */ > > > +static inline uint16_t __rte_pktmbuf_detach_on_free(struct rte_mbuf > > > +*m) > > > > I think int would be better than uint16_t > > > > > +{ > > > + if (RTE_MBUF_HAS_EXTBUF(m)) { > > > + uint32_t flags = 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 = EXT_ATTACHED_MBUF; > > > + shinfo = m->shinfo; > > > + /* Optimize for performance - do not dec/reinit */ > > > + if (likely(rte_mbuf_ext_refcnt_read(shinfo) == 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; > > > +} > > > > I don't think the API comment really reflects what is done in this function. In > > my understanding, the detach() operation does nothing on an extmem > > pinned mbuf. So detach() is probably not the proper name. > > > > What about something like this instead: > > > > /* [...]. > > * 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; > > > > /* Clear flags, mbuf is being freed. */ > > m->ol_flags = EXT_ATTACHED_MBUF; > > shinfo = m->shinfo; > > > > /* Optimize for performance - do not dec/reinit */ > > if (likely(rte_mbuf_ext_refcnt_read(shinfo) == 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; > > } > > > > static __rte_always_inline struct rte_mbuf * rte_pktmbuf_prefree_seg(struct > > rte_mbuf *m) { > > __rte_mbuf_sanity_check(m, 0); > > > > if (likely(rte_mbuf_refcnt_read(m) == 1)) { > > > > 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) ... > > > > > > (just quickly tested) > > > > 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 might affect > the performance for indirect packets (and it does not matter for packets with external > buffers). My approach updates the prefree routine for the packets with > external buffers only, keeping intact the handling for all other mbuf types. maybe just change the test to this? if (!RTE_MBUF_HAS_EXTBUF(m) || !RTE_MBUF_HAS_PINNED_EXTBUF(m)) if you prefer, test can be moved in __rte_pktmbuf_pinned_ext_buf_decref(): if (!RTE_MBUF_HAS_EXTBUF(m) || !RTE_MBUF_HAS_PINNED_EXTBUF(m)) return 0; But my preference would go to the 1st one. The root of my comment was more about the naming, I don't think the function should be something_detach() because it would not detach anything in case of ext mem pinned buffer. > > > > > > + > > > +/** > > > * 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) == 1)) { > > > > > > if (!RTE_MBUF_DIRECT(m)) > > > - rte_pktmbuf_detach(m); > > > + if (__rte_pktmbuf_detach_on_free(m)) > > > + return NULL; > > > > > > if (m->next != NULL) { > > > m->next = NULL; > > > @@ -1210,7 +1290,8 @@ static inline void rte_pktmbuf_detach(struct > > rte_mbuf *m) > > > } else if (__rte_mbuf_refcnt_update(m, -1) == 0) { > > > > > > if (!RTE_MBUF_DIRECT(m)) > > > - rte_pktmbuf_detach(m); > > > + if (__rte_pktmbuf_detach_on_free(m)) > > > + return NULL; > > > > > > if (m->next != NULL) { > > > m->next = NULL; > > > -- > > > 1.8.3.1 > > > >