[PATCH 0/3] net/hns3: bugfix on hns3

[PATCH 0/3] net/hns3: bugfix on hns3

[Jie Hai]

The patchset fixes some codes.

Dengdui Huang (1):
  net/hns3: fix simple Tx path incorrect free the mbuf

Jie Hai (2):
  net/hns3: remove pvid info dump for VF
  net/hns3: rename RAS module

 drivers/net/hns3/hns3_dump.c | 4 ++++
 drivers/net/hns3/hns3_intr.c | 4 ++--
 drivers/net/hns3/hns3_intr.h | 2 +-
 drivers/net/hns3/hns3_rxtx.c | 2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.22.0

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

[Jie Hai]

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>
---
 drivers/net/hns3/hns3_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index 03bbbc435fac..09e39cb673cb 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -4006,7 +4006,7 @@ hns3_tx_free_buffer_simple(struct hns3_tx_queue *txq)
 		for (i = 0; i < txq->tx_rs_thresh; i++)
 			rte_prefetch0((tx_entry + i)->mbuf);
 		for (i = 0; i < txq->tx_rs_thresh; i++, tx_entry++) {
-			rte_mempool_put(tx_entry->mbuf->pool, tx_entry->mbuf);
+			rte_pktmbuf_free_seg(tx_entry->mbuf);
 			tx_entry->mbuf = NULL;
 		}
 
-- 
2.22.0

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

[Stephen Hemminger]

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?

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

[Jie Hai]

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

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

[Stephen Hemminger]

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

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.

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

[huangdengdui]

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);
}

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.

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

[Stephen Hemminger]

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.

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

[huangdengdui]

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

[PATCH 2/3] net/hns3: remove pvid info dump for VF

[Jie Hai]

Since the pvid status obtained from kernel varies on different
platform, and the pvid of VF can be accessed by 'ip link show'
command, so remove it in case of misunderstanding.

Fixes: 871e5a4f881b ("net/hns3: dump VLAN configuration info")
Cc: stable@dpdk.org

Signed-off-by: Jie Hai <haijie1@huawei.com>
---
 drivers/net/hns3/hns3_dump.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/hns3/hns3_dump.c b/drivers/net/hns3/hns3_dump.c
index 738dcb0c42fc..f21d32e6a2b5 100644
--- a/drivers/net/hns3/hns3_dump.c
+++ b/drivers/net/hns3/hns3_dump.c
@@ -693,6 +693,10 @@ hns3_get_vlan_tx_offload_cfg(FILE *file, struct hns3_hw *hw)
 static void
 hns3_get_port_pvid_info(FILE *file, struct hns3_hw *hw)
 {
+	struct hns3_adapter *hns = HNS3_DEV_HW_TO_ADAPTER(hw);
+	if (hns->is_vf)
+		return;
+
 	fprintf(file, "  - pvid status: %s\n",
 		hw->port_base_vlan_cfg.state ? "On" : "Off");
 }
-- 
2.22.0

[PATCH 3/3] net/hns3: rename RAS module

[Jie Hai]

Rename ROH_MAC module as HIMAC to avoid misunderstandings.

Fixes: 1c1eb759e9d7 ("net/hns3: support RAS process in Kunpeng 930")
Cc: stable@dpdk.org

Signed-off-by: Jie Hai <haijie1@huawei.com>
---
 drivers/net/hns3/hns3_intr.c | 4 ++--
 drivers/net/hns3/hns3_intr.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/hns3/hns3_intr.c b/drivers/net/hns3/hns3_intr.c
index 2de2b86b0297..db5c84061ca3 100644
--- a/drivers/net/hns3/hns3_intr.c
+++ b/drivers/net/hns3/hns3_intr.c
@@ -1432,8 +1432,8 @@ static const struct hns3_hw_mod_name hns3_hw_module_name[] = {
 		.module_name = MODULE_MASTER,
 		.msg = "MODULE_MASTER"
 	}, {
-		.module_name = MODULE_ROH_MAC,
-		.msg = "MODULE_ROH_MAC"
+		.module_name = MODULE_HIMAC,
+		.msg = "MODULE_HIMAC"
 	}
 };
 
diff --git a/drivers/net/hns3/hns3_intr.h b/drivers/net/hns3/hns3_intr.h
index 1edb07de361b..e88b00c9c986 100644
--- a/drivers/net/hns3/hns3_intr.h
+++ b/drivers/net/hns3/hns3_intr.h
@@ -104,7 +104,7 @@ enum hns3_mod_name_list {
 	MODULE_RCB_TX,
 	MODULE_TXDMA,
 	MODULE_MASTER,
-	MODULE_ROH_MAC,
+	MODULE_HIMAC,
 };
 
 enum hns3_err_type_list {
-- 
2.22.0

Re: [PATCH 0/3] net/hns3: bugfix on hns3

[Stephen Hemminger]

On Mon, 30 Dec 2024 14:54:02 +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>
> CC: <lihuisong@huawei.com>, <fengchengwen@huawei.com>, <haijie1@huawei.com>,  <huangdengdui@huawei.com>
> Subject: [PATCH 0/3] net/hns3: bugfix on hns3
> Date: Mon, 30 Dec 2024 14:54:02 +0800
> X-Mailer: git-send-email 2.22.0
> 
> The patchset fixes some codes.
> 
> Dengdui Huang (1):
>   net/hns3: fix simple Tx path incorrect free the mbuf
> 
> Jie Hai (2):
>   net/hns3: remove pvid info dump for VF
>   net/hns3: rename RAS module
> 
>  drivers/net/hns3/hns3_dump.c | 4 ++++
>  drivers/net/hns3/hns3_intr.c | 4 ++--
>  drivers/net/hns3/hns3_intr.h | 2 +-
>  drivers/net/hns3/hns3_rxtx.c | 2 +-
>  4 files changed, 8 insertions(+), 4 deletions(-)


While reviewing these, I noticed that hns3 driver is open coding its own version
of rte_pktmbuf_alloc(). That was a mistake, if a future changes to rte_mbuf.c change
the allocation logic, the driver will break.