[PATCH] net/bonding: fix bond startup failure when NUMA is -1

[PATCH] net/bonding: fix bond startup failure when NUMA is -1

[Chaoyong He]

From: Zerun Fu <zerun.fu@corigine.com>

After the mainline Linux kernel commit
"fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
of partly creating a node in acpi_map_pxm_to_online_node) by
Jonathan Cameron. When the system does not support NUMA architecture,
the "socket_id" is expected to be -1. The valid "socket_id" in
BOND PMD is greater than or equal to zero. So it will cause an error
when DPDK checks the validity of the "socket_id" when starting the
bond. This commit can fix this bug.

Fixes: f294e04851fd ("net/bonding: fix socket ID check")
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
---
 drivers/net/bonding/rte_eth_bond_args.c | 6 ++++++
 drivers/net/bonding/rte_eth_bond_pmd.c  | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index 6553166f5c..c137efd55f 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -212,6 +212,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
 	if (*endptr != 0 || errno != 0)
 		return -1;
 
+	/* SOCKET_ID_ANY also consider a valid socket id */
+	if ((int8_t)socket_id == SOCKET_ID_ANY) {
+		*(int *)extra_args = SOCKET_ID_ANY;
+		return 0;
+	}
+
 	/* validate socket id value */
 	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {
 		*(int *)extra_args = (int)socket_id;
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index f0c4f7d26b..390a5b4271 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -3604,7 +3604,7 @@ static int
 bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
 {
 	const char *name = rte_vdev_device_name(dev);
-	uint8_t socket_id = dev->device.numa_node;
+	int socket_id = dev->device.numa_node;
 	struct bond_dev_private *internals = NULL;
 	struct rte_eth_dev *eth_dev = NULL;
 	uint32_t vlan_filter_bmp_size;
-- 
2.39.1

Re: [PATCH] net/bonding: fix bond startup failure when NUMA is -1

[humin (Q)]

Hi,


在 2023/6/16 11:20, Chaoyong He 写道:
> From: Zerun Fu <zerun.fu@corigine.com>
>
> After the mainline Linux kernel commit
> "fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
> of partly creating a node in acpi_map_pxm_to_online_node) by
> Jonathan Cameron. When the system does not support NUMA architecture,
> the "socket_id" is expected to be -1. The valid "socket_id" in
> BOND PMD is greater than or equal to zero. So it will cause an error
> when DPDK checks the validity of the "socket_id" when starting the
> bond. This commit can fix this bug.
>
> Fixes: f294e04851fd ("net/bonding: fix socket ID check")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> ---
>   drivers/net/bonding/rte_eth_bond_args.c | 6 ++++++
>   drivers/net/bonding/rte_eth_bond_pmd.c  | 2 +-
>   2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
> index 6553166f5c..c137efd55f 100644
> --- a/drivers/net/bonding/rte_eth_bond_args.c
> +++ b/drivers/net/bonding/rte_eth_bond_args.c
> @@ -212,6 +212,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
>   	if (*endptr != 0 || errno != 0)
>   		return -1;
>   
> +	/* SOCKET_ID_ANY also consider a valid socket id */
> +	if ((int8_t)socket_id == SOCKET_ID_ANY) {
> +		*(int *)extra_args = SOCKET_ID_ANY;
> +		return 0;
> +	}
> +
>   	/* validate socket id value */
>   	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {
>   		*(int *)extra_args = (int)socket_id;
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index f0c4f7d26b..390a5b4271 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -3604,7 +3604,7 @@ static int
>   bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
>   {
>   	const char *name = rte_vdev_device_name(dev);
> -	uint8_t socket_id = dev->device.numa_node;
> +	int socket_id = dev->device.numa_node;

Well,  other point should be also modified, like :

***

"socket %u.",    name, bonding_mode, socket_id);

***

%u -- > %d.


BTW,  I think  there is  no need  to add args like "socket_id=-1..." if 
we know this server does not support NUMA.

Default socket id is -1, so this is meaningless.



>   	struct bond_dev_private *internals = NULL;
>   	struct rte_eth_dev *eth_dev = NULL;
>   	uint32_t vlan_filter_bmp_size;

RE: [PATCH] net/bonding: fix bond startup failure when NUMA is -1

[Chaoyong He]

> 在 2023/6/16 11:20, Chaoyong He 写道:
> > From: Zerun Fu <zerun.fu@corigine.com>
> >
> > After the mainline Linux kernel commit
> > "fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side
> effect
> > of partly creating a node in acpi_map_pxm_to_online_node) by Jonathan
> > Cameron. When the system does not support NUMA architecture, the
> > "socket_id" is expected to be -1. The valid "socket_id" in BOND PMD is
> > greater than or equal to zero. So it will cause an error when DPDK
> > checks the validity of the "socket_id" when starting the bond. This
> > commit can fix this bug.
> >
> > Fixes: f294e04851fd ("net/bonding: fix socket ID check")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> > Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> > Reviewed-by: Long Wu <long.wu@corigine.com>
> > ---
> >   drivers/net/bonding/rte_eth_bond_args.c | 6 ++++++
> >   drivers/net/bonding/rte_eth_bond_pmd.c  | 2 +-
> >   2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/bonding/rte_eth_bond_args.c
> > b/drivers/net/bonding/rte_eth_bond_args.c
> > index 6553166f5c..c137efd55f 100644
> > --- a/drivers/net/bonding/rte_eth_bond_args.c
> > +++ b/drivers/net/bonding/rte_eth_bond_args.c
> > @@ -212,6 +212,12 @@ bond_ethdev_parse_socket_id_kvarg(const char
> *key __rte_unused,
> >       if (*endptr != 0 || errno != 0)
> >               return -1;
> >
> > +     /* SOCKET_ID_ANY also consider a valid socket id */
> > +     if ((int8_t)socket_id == SOCKET_ID_ANY) {
> > +             *(int *)extra_args = SOCKET_ID_ANY;
> > +             return 0;
> > +     }
> > +
> >       /* validate socket id value */
> >       if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {
> >               *(int *)extra_args = (int)socket_id; diff --git
> > a/drivers/net/bonding/rte_eth_bond_pmd.c
> > b/drivers/net/bonding/rte_eth_bond_pmd.c
> > index f0c4f7d26b..390a5b4271 100644
> > --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > @@ -3604,7 +3604,7 @@ static int
> >   bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
> >   {
> >       const char *name = rte_vdev_device_name(dev);
> > -     uint8_t socket_id = dev->device.numa_node;
> > +     int socket_id = dev->device.numa_node;
> 
> Well,  other point should be also modified, like :
> 
> ***
> 
> "socket %u.",    name, bonding_mode, socket_id);
> 
> ***
> 
> %u -- > %d.

Okay, I will send a v2 patch fix this.

> 
> BTW,  I think  there is  no need  to add args like "socket_id=-1..." if we know
> this server does not support NUMA.
> 
> Default socket id is -1, so this is meaningless.
> 

We found this bug when running 'dperf' app, and it is the 'dperf' app add this 'socket_id=-1' args.
Maybe the 'dperf' app should change its logic?
Please help correct me if I misunderstood, thanks.

> >       struct bond_dev_private *internals = NULL;
> >       struct rte_eth_dev *eth_dev = NULL;
> >       uint32_t vlan_filter_bmp_size;

[PATCH] net/bonding: fix bond startup failure when NUMA is -1

[Chaoyong He]

From: Zerun Fu <zerun.fu@corigine.com>

After the mainline Linux kernel commit
"fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
of partly creating a node in acpi_map_pxm_to_online_node) by
Jonathan Cameron. When the system does not support NUMA architecture,
the "socket_id" is expected to be -1. The valid "socket_id" in
BOND PMD is greater than or equal to zero. So it will cause an error
when DPDK checks the validity of the "socket_id" when starting the
bond. This commit can fix this bug.

Fixes: f294e04851fd ("net/bonding: fix socket ID check")
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
---
 drivers/net/bonding/rte_eth_bond_args.c | 6 ++++++
 drivers/net/bonding/rte_eth_bond_pmd.c  | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index 6553166f5c..c137efd55f 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -212,6 +212,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
 	if (*endptr != 0 || errno != 0)
 		return -1;
 
+	/* SOCKET_ID_ANY also consider a valid socket id */
+	if ((int8_t)socket_id == SOCKET_ID_ANY) {
+		*(int *)extra_args = SOCKET_ID_ANY;
+		return 0;
+	}
+
 	/* validate socket id value */
 	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {
 		*(int *)extra_args = (int)socket_id;
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index f0c4f7d26b..73205f78f4 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -3604,7 +3604,7 @@ static int
 bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
 {
 	const char *name = rte_vdev_device_name(dev);
-	uint8_t socket_id = dev->device.numa_node;
+	int socket_id = dev->device.numa_node;
 	struct bond_dev_private *internals = NULL;
 	struct rte_eth_dev *eth_dev = NULL;
 	uint32_t vlan_filter_bmp_size;
@@ -3806,7 +3806,7 @@ bond_probe(struct rte_vdev_device *dev)
 	port_id = bond_alloc(dev, bonding_mode);
 	if (port_id < 0) {
 		RTE_BOND_LOG(ERR, "Failed to create socket %s in mode %u on "
-				"socket %u.",	name, bonding_mode, socket_id);
+				"socket %d.",	name, bonding_mode, socket_id);
 		goto parse_error;
 	}
 	internals = rte_eth_devices[port_id].data->dev_private;
@@ -3831,7 +3831,7 @@ bond_probe(struct rte_vdev_device *dev)
 
 	rte_eth_dev_probing_finish(&rte_eth_devices[port_id]);
 	RTE_BOND_LOG(INFO, "Create bonded device %s on port %d in mode %u on "
-			"socket %u.",	name, port_id, bonding_mode, socket_id);
+			"socket %d.",	name, port_id, bonding_mode, socket_id);
 	return 0;
 
 parse_error:
-- 
2.39.1

[PATCH v3] net/bonding: fix bond startup failure when NUMA is -1

[Chaoyong He]

From: Zerun Fu <zerun.fu@corigine.com>

After the mainline Linux kernel commit
"fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
of partly creating a node in acpi_map_pxm_to_online_node) by
Jonathan Cameron. When the system does not support NUMA architecture,
the "socket_id" is expected to be -1. The valid "socket_id" in
BOND PMD is greater than or equal to zero. So it will cause an error
when DPDK checks the validity of the "socket_id" when starting the
bond. This commit can fix this bug.

Fixes: f294e04851fd ("net/bonding: fix socket ID check")
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
---
v2:
* Fix the print format string.
v3:
* Add the version number of patch.
---
 drivers/net/bonding/rte_eth_bond_args.c | 6 ++++++
 drivers/net/bonding/rte_eth_bond_pmd.c  | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index 6553166f5c..c137efd55f 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -212,6 +212,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
 	if (*endptr != 0 || errno != 0)
 		return -1;
 
+	/* SOCKET_ID_ANY also consider a valid socket id */
+	if ((int8_t)socket_id == SOCKET_ID_ANY) {
+		*(int *)extra_args = SOCKET_ID_ANY;
+		return 0;
+	}
+
 	/* validate socket id value */
 	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {
 		*(int *)extra_args = (int)socket_id;
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index f0c4f7d26b..73205f78f4 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -3604,7 +3604,7 @@ static int
 bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
 {
 	const char *name = rte_vdev_device_name(dev);
-	uint8_t socket_id = dev->device.numa_node;
+	int socket_id = dev->device.numa_node;
 	struct bond_dev_private *internals = NULL;
 	struct rte_eth_dev *eth_dev = NULL;
 	uint32_t vlan_filter_bmp_size;
@@ -3806,7 +3806,7 @@ bond_probe(struct rte_vdev_device *dev)
 	port_id = bond_alloc(dev, bonding_mode);
 	if (port_id < 0) {
 		RTE_BOND_LOG(ERR, "Failed to create socket %s in mode %u on "
-				"socket %u.",	name, bonding_mode, socket_id);
+				"socket %d.",	name, bonding_mode, socket_id);
 		goto parse_error;
 	}
 	internals = rte_eth_devices[port_id].data->dev_private;
@@ -3831,7 +3831,7 @@ bond_probe(struct rte_vdev_device *dev)
 
 	rte_eth_dev_probing_finish(&rte_eth_devices[port_id]);
 	RTE_BOND_LOG(INFO, "Create bonded device %s on port %d in mode %u on "
-			"socket %u.",	name, port_id, bonding_mode, socket_id);
+			"socket %d.",	name, port_id, bonding_mode, socket_id);
 	return 0;
 
 parse_error:
-- 
2.39.1

Re: [PATCH] net/bonding: fix bond startup failure when NUMA is -1

[humin (Q)]

在 2023/6/16 14:08, Chaoyong He 写道:
>> 在 2023/6/16 11:20, Chaoyong He 写道:
>>> From: Zerun Fu <zerun.fu@corigine.com>
>>>
>>> After the mainline Linux kernel commit
>>> "fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side
>> effect
>>> of partly creating a node in acpi_map_pxm_to_online_node) by Jonathan
>>> Cameron. When the system does not support NUMA architecture, the
>>> "socket_id" is expected to be -1. The valid "socket_id" in BOND PMD is
>>> greater than or equal to zero. So it will cause an error when DPDK
>>> checks the validity of the "socket_id" when starting the bond. This
>>> commit can fix this bug.
>>>
>>> Fixes: f294e04851fd ("net/bonding: fix socket ID check")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
>>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
>>> Reviewed-by: Long Wu <long.wu@corigine.com>
>>> ---
>>>    drivers/net/bonding/rte_eth_bond_args.c | 6 ++++++
>>>    drivers/net/bonding/rte_eth_bond_pmd.c  | 2 +-
>>>    2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/bonding/rte_eth_bond_args.c
>>> b/drivers/net/bonding/rte_eth_bond_args.c
>>> index 6553166f5c..c137efd55f 100644
>>> --- a/drivers/net/bonding/rte_eth_bond_args.c
>>> +++ b/drivers/net/bonding/rte_eth_bond_args.c
>>> @@ -212,6 +212,12 @@ bond_ethdev_parse_socket_id_kvarg(const char
>> *key __rte_unused,
>>>        if (*endptr != 0 || errno != 0)
>>>                return -1;
>>>
>>> +     /* SOCKET_ID_ANY also consider a valid socket id */
>>> +     if ((int8_t)socket_id == SOCKET_ID_ANY) {
>>> +             *(int *)extra_args = SOCKET_ID_ANY;
>>> +             return 0;
>>> +     }
>>> +
>>>        /* validate socket id value */
>>>        if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {
>>>                *(int *)extra_args = (int)socket_id; diff --git
>>> a/drivers/net/bonding/rte_eth_bond_pmd.c
>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> index f0c4f7d26b..390a5b4271 100644
>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> @@ -3604,7 +3604,7 @@ static int
>>>    bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
>>>    {
>>>        const char *name = rte_vdev_device_name(dev);
>>> -     uint8_t socket_id = dev->device.numa_node;
>>> +     int socket_id = dev->device.numa_node;
>> Well,  other point should be also modified, like :
>>
>> ***
>>
>> "socket %u.",    name, bonding_mode, socket_id);
>>
>> ***
>>
>> %u -- > %d.
> Okay, I will send a v2 patch fix this.
>
>> BTW,  I think  there is  no need  to add args like "socket_id=-1..." if we know
>> this server does not support NUMA.
>>
>> Default socket id is -1, so this is meaningless.
>>
> We found this bug when running 'dperf' app, and it is the 'dperf' app add this 'socket_id=-1' args.
> Maybe the 'dperf' app should change its logic?
> Please help correct me if I misunderstood, thanks.

I agree with your patch.

What I mentioned is just for further discussion.


>
>>>        struct bond_dev_private *internals = NULL;
>>>        struct rte_eth_dev *eth_dev = NULL;
>>>        uint32_t vlan_filter_bmp_size;

Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1

[humin (Q)]

Hi,

在 2023/6/16 15:20, Chaoyong He 写道:
> From: Zerun Fu <zerun.fu@corigine.com>
>
> After the mainline Linux kernel commit
> "fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
> of partly creating a node in acpi_map_pxm_to_online_node) by
> Jonathan Cameron. When the system does not support NUMA architecture,
> the "socket_id" is expected to be -1. The valid "socket_id" in
> BOND PMD is greater than or equal to zero. So it will cause an error
> when DPDK checks the validity of the "socket_id" when starting the
> bond. This commit can fix this bug.
>
> Fixes: f294e04851fd ("net/bonding: fix socket ID check")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>

No need add your colleagues unless they "reviwed-by" through email-list.

> ---
> v2:
> * Fix the print format string.
> v3:
> * Add the version number of patch.
> ---
>   drivers/net/bonding/rte_eth_bond_args.c | 6 ++++++
>   drivers/net/bonding/rte_eth_bond_pmd.c  | 6 +++---
>   2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
> index 6553166f5c..c137efd55f 100644
> --- a/drivers/net/bonding/rte_eth_bond_args.c
> +++ b/drivers/net/bonding/rte_eth_bond_args.c
> @@ -212,6 +212,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
>   	if (*endptr != 0 || errno != 0)
>   		return -1;
>   
> +	/* SOCKET_ID_ANY also consider a valid socket id */
> +	if ((int8_t)socket_id == SOCKET_ID_ANY) {
> +		*(int *)extra_args = SOCKET_ID_ANY;
> +		return 0;
> +	}
> +
>   	/* validate socket id value */
>   	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {
>   		*(int *)extra_args = (int)socket_id;
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index f0c4f7d26b..73205f78f4 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -3604,7 +3604,7 @@ static int
>   bond_alloc(struct rte_vdev_device *dev, uint8_t mode)
>   {
>   	const char *name = rte_vdev_device_name(dev);
> -	uint8_t socket_id = dev->device.numa_node;
> +	int socket_id = dev->device.numa_node;
>   	struct bond_dev_private *internals = NULL;
>   	struct rte_eth_dev *eth_dev = NULL;
>   	uint32_t vlan_filter_bmp_size;
> @@ -3806,7 +3806,7 @@ bond_probe(struct rte_vdev_device *dev)
>   	port_id = bond_alloc(dev, bonding_mode);
>   	if (port_id < 0) {
>   		RTE_BOND_LOG(ERR, "Failed to create socket %s in mode %u on "
> -				"socket %u.",	name, bonding_mode, socket_id);
> +				"socket %d.",	name, bonding_mode, socket_id);
>   		goto parse_error;
>   	}
>   	internals = rte_eth_devices[port_id].data->dev_private;
> @@ -3831,7 +3831,7 @@ bond_probe(struct rte_vdev_device *dev)
>   
>   	rte_eth_dev_probing_finish(&rte_eth_devices[port_id]);
>   	RTE_BOND_LOG(INFO, "Create bonded device %s on port %d in mode %u on "
> -			"socket %u.",	name, port_id, bonding_mode, socket_id);
> +			"socket %d.",	name, port_id, bonding_mode, socket_id);
>   	return 0;
>   
>   parse_error:

Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1

[Ferruh Yigit]

On 6/16/2023 1:00 PM, humin (Q) wrote:
> Hi,
> 
> 在 2023/6/16 15:20, Chaoyong He 写道:
>> From: Zerun Fu <zerun.fu@corigine.com>
>>
>> After the mainline Linux kernel commit
>> "fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
>> of partly creating a node in acpi_map_pxm_to_online_node) by
>> Jonathan Cameron. When the system does not support NUMA architecture,
>> the "socket_id" is expected to be -1. The valid "socket_id" in
>> BOND PMD is greater than or equal to zero. So it will cause an error
>> when DPDK checks the validity of the "socket_id" when starting the
>> bond. This commit can fix this bug.
>>
>> Fixes: f294e04851fd ("net/bonding: fix socket ID check")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
>> Reviewed-by: Long Wu <long.wu@corigine.com>
> 
> No need add your colleagues unless they "reviwed-by" through email-list.
> 

Hi Connor,

This is done time to time, if code is already internally reviewed, send
review/ack tags within the patch, to reduce noise in the mail list.

It looks like there were additional reviewers of code, which is good,
but it requires maintainers' (you and Chas) ack to get accepted.

Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1

[humin (Q)]

在 2023/6/19 16:57, Ferruh Yigit 写道:
> On 6/16/2023 1:00 PM, humin (Q) wrote:
>> Hi,
>>
>> 在 2023/6/16 15:20, Chaoyong He 写道:
>>> From: Zerun Fu <zerun.fu@corigine.com>
>>>
>>> After the mainline Linux kernel commit
>>> "fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
>>> of partly creating a node in acpi_map_pxm_to_online_node) by
>>> Jonathan Cameron. When the system does not support NUMA architecture,
>>> the "socket_id" is expected to be -1. The valid "socket_id" in
>>> BOND PMD is greater than or equal to zero. So it will cause an error
>>> when DPDK checks the validity of the "socket_id" when starting the
>>> bond. This commit can fix this bug.
>>>
>>> Fixes: f294e04851fd ("net/bonding: fix socket ID check")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
>>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
>>> Reviewed-by: Long Wu <long.wu@corigine.com>
>> No need add your colleagues unless they "reviwed-by" through email-list.
>>
> Hi Connor,
>
> This is done time to time, if code is already internally reviewed, send
> review/ack tags within the patch, to reduce noise in the mail list.

ok.

>
> It looks like there were additional reviewers of code, which is good,
> but it requires maintainers' (you and Chas) ack to get accepted.

Acked-by: Min Hu (Connor) <humin29@huawei.com>

Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1

[Ferruh Yigit]

On 6/20/2023 3:53 AM, humin (Q) wrote:
> 
> 在 2023/6/19 16:57, Ferruh Yigit 写道:
>> On 6/16/2023 1:00 PM, humin (Q) wrote:
>>> Hi,
>>>
>>> 在 2023/6/16 15:20, Chaoyong He 写道:
>>>> From: Zerun Fu <zerun.fu@corigine.com>
>>>>
>>>> After the mainline Linux kernel commit
>>>> "fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
>>>> of partly creating a node in acpi_map_pxm_to_online_node) by
>>>> Jonathan Cameron. When the system does not support NUMA architecture,
>>>> the "socket_id" is expected to be -1. The valid "socket_id" in
>>>> BOND PMD is greater than or equal to zero. So it will cause an error
>>>> when DPDK checks the validity of the "socket_id" when starting the
>>>> bond. This commit can fix this bug.
>>>>
>>>> Fixes: f294e04851fd ("net/bonding: fix socket ID check")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
>>>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
>>>> Reviewed-by: Long Wu <long.wu@corigine.com>
>>> No need add your colleagues unless they "reviwed-by" through email-list.
>>>
>> Hi Connor,
>>
>> This is done time to time, if code is already internally reviewed, send
>> review/ack tags within the patch, to reduce noise in the mail list.
> 
> ok.
> 
>>
>> It looks like there were additional reviewers of code, which is good,
>> but it requires maintainers' (you and Chas) ack to get accepted.
> 
> Acked-by: Min Hu (Connor) <humin29@huawei.com>
> 
> 

Applied to dpdk-next-net/main, thanks.

Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1

[Niklas Söderlund]

Hi Connor and Ferruh,

On 2023-06-19 09:57:17 +0100, Ferruh Yigit wrote:
> On 6/16/2023 1:00 PM, humin (Q) wrote:
> > Hi,
> > 
> > 在 2023/6/16 15:20, Chaoyong He 写道:
> >> From: Zerun Fu <zerun.fu@corigine.com>
> >>
> >> After the mainline Linux kernel commit
> >> "fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
> >> of partly creating a node in acpi_map_pxm_to_online_node) by
> >> Jonathan Cameron. When the system does not support NUMA architecture,
> >> the "socket_id" is expected to be -1. The valid "socket_id" in
> >> BOND PMD is greater than or equal to zero. So it will cause an error
> >> when DPDK checks the validity of the "socket_id" when starting the
> >> bond. This commit can fix this bug.
> >>
> >> Fixes: f294e04851fd ("net/bonding: fix socket ID check")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> >> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> >> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> >> Reviewed-by: Long Wu <long.wu@corigine.com>
> > 
> > No need add your colleagues unless they "reviwed-by" through email-list.
> > 
> 
> Hi Connor,
> 
> This is done time to time, if code is already internally reviewed, send
> review/ack tags within the patch, to reduce noise in the mail list.

This is the reason why patches from us usually have 1 or 2 R-b tags when 
we post to the list. We have an internal review and CI pipeline we run 
patches thru to reduce the noise at the list and to not waste upstream 
review resources.

We follow the DPDK workflow internally before we submit patches to the 
public mailing list. I hope we can continue to do so and add R-b tags, 
as they represent real effort by the developers.

> 
> It looks like there were additional reviewers of code, which is good,
> but it requires maintainers' (you and Chas) ack to get accepted.
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1

[humin (Q)]

Hi, Niklas, Ferruh,

在 2023/6/20 19:03, Niklas Söderlund 写道:
> Hi Connor and Ferruh,
>
> On 2023-06-19 09:57:17 +0100, Ferruh Yigit wrote:
>> On 6/16/2023 1:00 PM, humin (Q) wrote:
>>> Hi,
>>>
>>> 在 2023/6/16 15:20, Chaoyong He 写道:
>>>> From: Zerun Fu <zerun.fu@corigine.com>
>>>>
>>>> After the mainline Linux kernel commit
>>>> "fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
>>>> of partly creating a node in acpi_map_pxm_to_online_node) by
>>>> Jonathan Cameron. When the system does not support NUMA architecture,
>>>> the "socket_id" is expected to be -1. The valid "socket_id" in
>>>> BOND PMD is greater than or equal to zero. So it will cause an error
>>>> when DPDK checks the validity of the "socket_id" when starting the
>>>> bond. This commit can fix this bug.
>>>>
>>>> Fixes: f294e04851fd ("net/bonding: fix socket ID check")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
>>>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
>>>> Reviewed-by: Long Wu <long.wu@corigine.com>
>>> No need add your colleagues unless they "reviwed-by" through email-list.
>>>
>> Hi Connor,
>>
>> This is done time to time, if code is already internally reviewed, send
>> review/ack tags within the patch, to reduce noise in the mail list.
> This is the reason why patches from us usually have 1 or 2 R-b tags when
> we post to the list. We have an internal review and CI pipeline we run
> patches thru to reduce the noise at the list and to not waste upstream
> review resources.
>
> We follow the DPDK workflow internally before we submit patches to the
> public mailing list. I hope we can continue to do so and add R-b tags,
> as they represent real effort by the developers.

Actually,  this is an interesting story.

The reason why I said so is I met same circumstances a few years ago.

Then I sent one patch with R-b tags which added internally by my colleagues.

Someone from mail-list said this was not right.  From then on, every time I

send patches, I will delete R-b tags and send to mail-list.


>
>> It looks like there were additional reviewers of code, which is good,
>> but it requires maintainers' (you and Chas) ack to get accepted.
>>

Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1

[Ferruh Yigit]

On 6/20/2023 2:15 PM, humin (Q) wrote:
> Hi, Niklas, Ferruh,
> 
> 在 2023/6/20 19:03, Niklas Söderlund 写道:
>> Hi Connor and Ferruh,
>>
>> On 2023-06-19 09:57:17 +0100, Ferruh Yigit wrote:
>>> On 6/16/2023 1:00 PM, humin (Q) wrote:
>>>> Hi,
>>>>
>>>> 在 2023/6/16 15:20, Chaoyong He 写道:
>>>>> From: Zerun Fu <zerun.fu@corigine.com>
>>>>>
>>>>> After the mainline Linux kernel commit
>>>>> "fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
>>>>> of partly creating a node in acpi_map_pxm_to_online_node) by
>>>>> Jonathan Cameron. When the system does not support NUMA architecture,
>>>>> the "socket_id" is expected to be -1. The valid "socket_id" in
>>>>> BOND PMD is greater than or equal to zero. So it will cause an error
>>>>> when DPDK checks the validity of the "socket_id" when starting the
>>>>> bond. This commit can fix this bug.
>>>>>
>>>>> Fixes: f294e04851fd ("net/bonding: fix socket ID check")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
>>>>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>>>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
>>>>> Reviewed-by: Long Wu <long.wu@corigine.com>
>>>> No need add your colleagues unless they "reviwed-by" through
>>>> email-list.
>>>>
>>> Hi Connor,
>>>
>>> This is done time to time, if code is already internally reviewed, send
>>> review/ack tags within the patch, to reduce noise in the mail list.
>> This is the reason why patches from us usually have 1 or 2 R-b tags when
>> we post to the list. We have an internal review and CI pipeline we run
>> patches thru to reduce the noise at the list and to not waste upstream
>> review resources.
>>
>> We follow the DPDK workflow internally before we submit patches to the
>> public mailing list. I hope we can continue to do so and add R-b tags,
>> as they represent real effort by the developers.
> 
> Actually,  this is an interesting story.
> The reason why I said so is I met same circumstances a few years ago.
> Then I sent one patch with R-b tags which added internally by my
> colleagues.
> 
> Someone from mail-list said this was not right.  From then on, every time I
> send patches, I will delete R-b tags and send to mail-list.
> 

For a driver, vendor and maintainers are experts and probably internal
reviews are good enough. Most of the times details are not interesting
to the rest of the community.

But for code that impact multiple vendors, like libraries, it is good to
have public reviews to share reasoning and updates and record them for
future.

Sometimes for this code that impact multiple vendor, if the author and
maintainer are from same company, we receive a patch with maintainer's
ack with it.
This raises questions on how much it is reviewed, how many internal
version it went through 1 or 11, or what was the design decision
reasoning etc.. If these concerns felt somehow, explicit acks from
maintainer requested time to time.

Or for me, some obvious mistakes in patch, with maintainer's ack already
embedded in it raises similar concerns and I request explicit ack.

But technically there is nothing preventing ack to be embedded into
patch itself.

Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1

[Thomas Monjalon]

20/06/2023 16:10, Ferruh Yigit:
> On 6/20/2023 2:15 PM, humin (Q) wrote:
> > Hi, Niklas, Ferruh,
> > 
> > 在 2023/6/20 19:03, Niklas Söderlund 写道:
> >> Hi Connor and Ferruh,
> >>
> >> On 2023-06-19 09:57:17 +0100, Ferruh Yigit wrote:
> >>> On 6/16/2023 1:00 PM, humin (Q) wrote:
> >>>> Hi,
> >>>>
> >>>> 在 2023/6/16 15:20, Chaoyong He 写道:
> >>>>> From: Zerun Fu <zerun.fu@corigine.com>
> >>>>>
> >>>>> After the mainline Linux kernel commit
> >>>>> "fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
> >>>>> of partly creating a node in acpi_map_pxm_to_online_node) by
> >>>>> Jonathan Cameron. When the system does not support NUMA architecture,
> >>>>> the "socket_id" is expected to be -1. The valid "socket_id" in
> >>>>> BOND PMD is greater than or equal to zero. So it will cause an error
> >>>>> when DPDK checks the validity of the "socket_id" when starting the
> >>>>> bond. This commit can fix this bug.
> >>>>>
> >>>>> Fixes: f294e04851fd ("net/bonding: fix socket ID check")
> >>>>> Cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> >>>>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> >>>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> >>>>> Reviewed-by: Long Wu <long.wu@corigine.com>
> >>>> No need add your colleagues unless they "reviwed-by" through
> >>>> email-list.
> >>>>
> >>> Hi Connor,
> >>>
> >>> This is done time to time, if code is already internally reviewed, send
> >>> review/ack tags within the patch, to reduce noise in the mail list.
> >> This is the reason why patches from us usually have 1 or 2 R-b tags when
> >> we post to the list. We have an internal review and CI pipeline we run
> >> patches thru to reduce the noise at the list and to not waste upstream
> >> review resources.
> >>
> >> We follow the DPDK workflow internally before we submit patches to the
> >> public mailing list. I hope we can continue to do so and add R-b tags,
> >> as they represent real effort by the developers.
> > 
> > Actually,  this is an interesting story.
> > The reason why I said so is I met same circumstances a few years ago.
> > Then I sent one patch with R-b tags which added internally by my
> > colleagues.
> > 
> > Someone from mail-list said this was not right.  From then on, every time I
> > send patches, I will delete R-b tags and send to mail-list.
> > 
> 
> For a driver, vendor and maintainers are experts and probably internal
> reviews are good enough. Most of the times details are not interesting
> to the rest of the community.
> 
> But for code that impact multiple vendors, like libraries, it is good to
> have public reviews to share reasoning and updates and record them for
> future.
> 
> Sometimes for this code that impact multiple vendor, if the author and
> maintainer are from same company, we receive a patch with maintainer's
> ack with it.
> This raises questions on how much it is reviewed, how many internal
> version it went through 1 or 11, or what was the design decision
> reasoning etc.. If these concerns felt somehow, explicit acks from
> maintainer requested time to time.
> 
> Or for me, some obvious mistakes in patch, with maintainer's ack already
> embedded in it raises similar concerns and I request explicit ack.
> 
> But technically there is nothing preventing ack to be embedded into
> patch itself.

Yes
If there is an internal review, you can embed the tag.

The decision of what is enough for a patch to be merged
is the role of the tree maintainer.
The component maintainer must make sure that the patches are well reviewed,
and the tree maintainer does a last check of trust & quality.

Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1

[humin (Q)]

在 2023/6/20 22:10, Ferruh Yigit 写道:
> On 6/20/2023 2:15 PM, humin (Q) wrote:
>> Hi, Niklas, Ferruh,
>>
>> 在 2023/6/20 19:03, Niklas Söderlund 写道:
>>> Hi Connor and Ferruh,
>>>
>>> On 2023-06-19 09:57:17 +0100, Ferruh Yigit wrote:
>>>> On 6/16/2023 1:00 PM, humin (Q) wrote:
>>>>> Hi,
>>>>>
>>>>> 在 2023/6/16 15:20, Chaoyong He 写道:
>>>>>> From: Zerun Fu <zerun.fu@corigine.com>
>>>>>>
>>>>>> After the mainline Linux kernel commit
>>>>>> "fe205d984e7730f4d21f6f8ebc60f0698404ac31" (ACPI: Remove side effect
>>>>>> of partly creating a node in acpi_map_pxm_to_online_node) by
>>>>>> Jonathan Cameron. When the system does not support NUMA architecture,
>>>>>> the "socket_id" is expected to be -1. The valid "socket_id" in
>>>>>> BOND PMD is greater than or equal to zero. So it will cause an error
>>>>>> when DPDK checks the validity of the "socket_id" when starting the
>>>>>> bond. This commit can fix this bug.
>>>>>>
>>>>>> Fixes: f294e04851fd ("net/bonding: fix socket ID check")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
>>>>>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>>>>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
>>>>>> Reviewed-by: Long Wu <long.wu@corigine.com>
>>>>> No need add your colleagues unless they "reviwed-by" through
>>>>> email-list.
>>>>>
>>>> Hi Connor,
>>>>
>>>> This is done time to time, if code is already internally reviewed, send
>>>> review/ack tags within the patch, to reduce noise in the mail list.
>>> This is the reason why patches from us usually have 1 or 2 R-b tags when
>>> we post to the list. We have an internal review and CI pipeline we run
>>> patches thru to reduce the noise at the list and to not waste upstream
>>> review resources.
>>>
>>> We follow the DPDK workflow internally before we submit patches to the
>>> public mailing list. I hope we can continue to do so and add R-b tags,
>>> as they represent real effort by the developers.
>> Actually,  this is an interesting story.
>> The reason why I said so is I met same circumstances a few years ago.
>> Then I sent one patch with R-b tags which added internally by my
>> colleagues.
>>
>> Someone from mail-list said this was not right.  From then on, every time I
>> send patches, I will delete R-b tags and send to mail-list.
>>
> For a driver, vendor and maintainers are experts and probably internal
> reviews are good enough. Most of the times details are not interesting
> to the rest of the community.
Ok, I see, thanks Ferruh.
> But for code that impact multiple vendors, like libraries, it is good to
> have public reviews to share reasoning and updates and record them for
> future.
>
> Sometimes for this code that impact multiple vendor, if the author and
> maintainer are from same company, we receive a patch with maintainer's
> ack with it.
> This raises questions on how much it is reviewed, how many internal
> version it went through 1 or 11, or what was the design decision
> reasoning etc.. If these concerns felt somehow, explicit acks from
> maintainer requested time to time.
>
> Or for me, some obvious mistakes in patch, with maintainer's ack already
> embedded in it raises similar concerns and I request explicit ack.
>
> But technically there is nothing preventing ack to be embedded into
> patch itself.
>