Re: [PATCH 1/3] net/hns3: fix simple Tx path incorrect free the mbuf

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 9 Jan 2025 15:43:12 +0800
huangdengdui <huangdengdui@huawei.com> wrote:

> On 2025/1/9 0:57, Stephen Hemminger wrote:
> > On Wed, 8 Jan 2025 10:40:43 +0800
> > Jie Hai <haijie1@huawei.com> wrote:
> >   
> >> On 2024/12/31 1:55, Stephen Hemminger wrote:  
> >>> On Mon, 30 Dec 2024 14:54:03 +0800
> >>> Jie Hai <haijie1@huawei.com> wrote:
> >>>     
> >>>> From: Jie Hai <haijie1@huawei.com>
> >>>> To: <dev@dpdk.org>, <thomas@monjalon.net>, <ferruh.yigit@amd.com>,  <david.marchand@redhat.com>, <andrew.rybchenko@oktetlabs.ru>, Chengwen Feng  <fengchengwen@huawei.com>, "Wei Hu (Xavier)" <xavier.huwei@huawei.com>,  Huisong Li <lihuisong@huawei.com>
> >>>> CC: <haijie1@huawei.com>, <huangdengdui@huawei.com>
> >>>> Subject: [PATCH 1/3] net/hns3: fix simple Tx path incorrect free the mbuf
> >>>> Date: Mon, 30 Dec 2024 14:54:03 +0800
> >>>> X-Mailer: git-send-email 2.22.0
> >>>>
> >>>> From: Dengdui Huang <huangdengdui@huawei.com>
> >>>>
> >>>> When RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE offload is not set,
> >>>> use rte_pktmbuf_free_seg() to free the mbuf.
> >>>>
> >>>> Fixes: 7ef933908f04 ("net/hns3: add simple Tx path")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> >>>> Signed-off-by: Jie Hai <haijie1@huawei.com>    
> >>>
> >>> What about the fast free case which is using rte_mempool_put_bulk when
> >>> it should use rte_pktmbuf_free_bulk instead?
> >>>
> >>>     
> >> Hi, Stephen Hemminger,
> >>
> >> During the fast free case, the performance of using
> >> rte_mempool_put_bulk is higher than that of using
> >> rte_pktmbuf_free_bulk because other references
> >> to mbuf do not need to be considered. So it's better
> >>   to not change.
> >>
> >> Thanks,
> >> Jie Hai  
> > 
> > The problem is that having an open coded version of this buried in
> > one driver is a long term potential proble>
> > If you really think that optimizing free like this is noticeable, then
> > why not make a new function "rte_pktmuf_fast_free_bulk" and put it in the
> > regular mbuf library.
> >   
> 
> Do you mean to add the following functions to the library?
> 
> void rte_pktmbuf_fast_free_bulk(struct rte_mbuf **mbufs, unsigned int count)
> {
> 	rte_mempool_put_bulk(mbufs[0]->pool, (void **)mbufs, count);
> }

Yes something like that. Or call it rte_mbuf_raw_free_bulk to align with
what rte_mbuf_raw_free(). And maybe add some debug assertions to make sure
that mbuf is not cloned, indirect or has refcnt.

The concern is that this optimization might put an mbuf in the pool
that has different properties than the normal free path.
And that all semantics of allocation/free should be in rte_mbuf code to
allow for future optimizations.


> 
> The driver uses rte_mempool_put_bulk only when the following conditions are met:
> 1. All mbufs comes from the same mempool
> 2. All mbufs have only one reference.
> 3. All mbufs have only one segment.
> So the rte_pktmbuf_fast_free_bulk function is just a wrapper around the rte_mempool_put_bulk function.