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

[huangdengdui <huangdengdui@huawei.com>]

On 2025/1/10 1:03, Stephen Hemminger wrote:
> 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.

"Morten Brørup" has submitted a patch to implement this function.[1]
Currently, multiple drivers use rte_mempool_put_bulk to release mbufs.
We can modify them later so that all drivers use rte_mbuf_fast_free_bulk to free mbufs.

The current patch is a function problem, which has caused troubles to hns3 users. Can we merge it first?

[1]https://inbox.dpdk.org/dev/20250114163951.125667-1-mb@smartsharesystems.com/

> 
> 
>>
>> 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.
>