patches for DPDK stable branches
 help / color / mirror / Atom feed
* [PATCH] net/bonding: fix bond startup failure when NUMA is -1
@ 2023-06-16  3:20 Chaoyong He
  2023-06-16  3:54 ` humin (Q)
  2023-06-16  7:15 ` Chaoyong He
  0 siblings, 2 replies; 15+ messages in thread
From: Chaoyong He @ 2023-06-16  3:20 UTC (permalink / raw)
  To: dev
  Cc: oss-drivers, niklas.soderlund, Zerun Fu, stable, Peng Zhang,
	Chaoyong He, Long Wu

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-16  3:20 [PATCH] net/bonding: fix bond startup failure when NUMA is -1 Chaoyong He
@ 2023-06-16  3:54 ` humin (Q)
  2023-06-16  6:08   ` Chaoyong He
  2023-06-16  7:15 ` Chaoyong He
  1 sibling, 1 reply; 15+ messages in thread
From: humin (Q) @ 2023-06-16  3:54 UTC (permalink / raw)
  To: Chaoyong He, dev
  Cc: oss-drivers, niklas.soderlund, Zerun Fu, stable, Peng Zhang, Long Wu

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;

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-16  3:54 ` humin (Q)
@ 2023-06-16  6:08   ` Chaoyong He
  2023-06-16 11:57     ` humin (Q)
  0 siblings, 1 reply; 15+ messages in thread
From: Chaoyong He @ 2023-06-16  6:08 UTC (permalink / raw)
  To: humin (Q), dev
  Cc: oss-drivers, Niklas Soderlund, Zerun Fu, stable, Nole Zhang, Long Wu

> 在 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;

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-16  3:20 [PATCH] net/bonding: fix bond startup failure when NUMA is -1 Chaoyong He
  2023-06-16  3:54 ` humin (Q)
@ 2023-06-16  7:15 ` Chaoyong He
  2023-06-16  7:20   ` [PATCH v3] " Chaoyong He
  1 sibling, 1 reply; 15+ messages in thread
From: Chaoyong He @ 2023-06-16  7:15 UTC (permalink / raw)
  To: dev
  Cc: oss-drivers, niklas.soderlund, Zerun Fu, stable, Peng Zhang,
	Chaoyong He, Long Wu

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-16  7:15 ` Chaoyong He
@ 2023-06-16  7:20   ` Chaoyong He
  2023-06-16 12:00     ` humin (Q)
  0 siblings, 1 reply; 15+ messages in thread
From: Chaoyong He @ 2023-06-16  7:20 UTC (permalink / raw)
  To: dev
  Cc: oss-drivers, niklas.soderlund, Zerun Fu, stable, Peng Zhang,
	Chaoyong He, Long Wu

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-16  6:08   ` Chaoyong He
@ 2023-06-16 11:57     ` humin (Q)
  0 siblings, 0 replies; 15+ messages in thread
From: humin (Q) @ 2023-06-16 11:57 UTC (permalink / raw)
  To: Chaoyong He, dev
  Cc: oss-drivers, Niklas Soderlund, Zerun Fu, stable, Nole Zhang, Long Wu


在 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;

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-16  7:20   ` [PATCH v3] " Chaoyong He
@ 2023-06-16 12:00     ` humin (Q)
  2023-06-19  8:57       ` Ferruh Yigit
  0 siblings, 1 reply; 15+ messages in thread
From: humin (Q) @ 2023-06-16 12:00 UTC (permalink / raw)
  To: Chaoyong He, dev
  Cc: oss-drivers, niklas.soderlund, Zerun Fu, stable, Peng Zhang, Long Wu

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:

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-16 12:00     ` humin (Q)
@ 2023-06-19  8:57       ` Ferruh Yigit
  2023-06-20  2:53         ` humin (Q)
  2023-06-20 11:03         ` Niklas Söderlund
  0 siblings, 2 replies; 15+ messages in thread
From: Ferruh Yigit @ 2023-06-19  8:57 UTC (permalink / raw)
  To: humin (Q), Chaoyong He, dev
  Cc: oss-drivers, niklas.soderlund, Zerun Fu, stable, Peng Zhang, Long Wu

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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-19  8:57       ` Ferruh Yigit
@ 2023-06-20  2:53         ` humin (Q)
  2023-06-20 10:18           ` Ferruh Yigit
  2023-06-20 11:03         ` Niklas Söderlund
  1 sibling, 1 reply; 15+ messages in thread
From: humin (Q) @ 2023-06-20  2:53 UTC (permalink / raw)
  To: Ferruh Yigit, Chaoyong He, dev
  Cc: oss-drivers, niklas.soderlund, Zerun Fu, stable, Peng Zhang, Long Wu


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



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-20  2:53         ` humin (Q)
@ 2023-06-20 10:18           ` Ferruh Yigit
  0 siblings, 0 replies; 15+ messages in thread
From: Ferruh Yigit @ 2023-06-20 10:18 UTC (permalink / raw)
  To: humin (Q), Chaoyong He, dev
  Cc: oss-drivers, niklas.soderlund, Zerun Fu, stable, Peng Zhang, Long Wu

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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-19  8:57       ` Ferruh Yigit
  2023-06-20  2:53         ` humin (Q)
@ 2023-06-20 11:03         ` Niklas Söderlund
  2023-06-20 13:15           ` humin (Q)
  1 sibling, 1 reply; 15+ messages in thread
From: Niklas Söderlund @ 2023-06-20 11:03 UTC (permalink / raw)
  To: Ferruh Yigit, humin (Q)
  Cc: Chaoyong He, dev, oss-drivers, Zerun Fu, stable, Peng Zhang, Long Wu

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-20 11:03         ` Niklas Söderlund
@ 2023-06-20 13:15           ` humin (Q)
  2023-06-20 14:10             ` Ferruh Yigit
  0 siblings, 1 reply; 15+ messages in thread
From: humin (Q) @ 2023-06-20 13:15 UTC (permalink / raw)
  To: Niklas Söderlund, Ferruh Yigit
  Cc: Chaoyong He, dev, oss-drivers, Zerun Fu, stable, Peng Zhang, Long Wu

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-20 13:15           ` humin (Q)
@ 2023-06-20 14:10             ` Ferruh Yigit
  2023-06-20 14:19               ` Thomas Monjalon
  2023-06-21  4:00               ` humin (Q)
  0 siblings, 2 replies; 15+ messages in thread
From: Ferruh Yigit @ 2023-06-20 14:10 UTC (permalink / raw)
  To: humin (Q), Niklas Söderlund
  Cc: Chaoyong He, dev, oss-drivers, Zerun Fu, stable, Peng Zhang,
	Long Wu, Thomas Monjalon, David Marchand

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.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-20 14:10             ` Ferruh Yigit
@ 2023-06-20 14:19               ` Thomas Monjalon
  2023-06-21  4:00               ` humin (Q)
  1 sibling, 0 replies; 15+ messages in thread
From: Thomas Monjalon @ 2023-06-20 14:19 UTC (permalink / raw)
  To: humin (Q), Niklas Söderlund, Ferruh Yigit
  Cc: Chaoyong He, dev, oss-drivers, Zerun Fu, stable, Peng Zhang,
	Long Wu, David Marchand

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.




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] net/bonding: fix bond startup failure when NUMA is -1
  2023-06-20 14:10             ` Ferruh Yigit
  2023-06-20 14:19               ` Thomas Monjalon
@ 2023-06-21  4:00               ` humin (Q)
  1 sibling, 0 replies; 15+ messages in thread
From: humin (Q) @ 2023-06-21  4:00 UTC (permalink / raw)
  To: Ferruh Yigit, Niklas Söderlund
  Cc: Chaoyong He, dev, oss-drivers, Zerun Fu, stable, Peng Zhang,
	Long Wu, Thomas Monjalon, David Marchand


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2023-06-21  4:00 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-16  3:20 [PATCH] net/bonding: fix bond startup failure when NUMA is -1 Chaoyong He
2023-06-16  3:54 ` humin (Q)
2023-06-16  6:08   ` Chaoyong He
2023-06-16 11:57     ` humin (Q)
2023-06-16  7:15 ` Chaoyong He
2023-06-16  7:20   ` [PATCH v3] " Chaoyong He
2023-06-16 12:00     ` humin (Q)
2023-06-19  8:57       ` Ferruh Yigit
2023-06-20  2:53         ` humin (Q)
2023-06-20 10:18           ` Ferruh Yigit
2023-06-20 11:03         ` Niklas Söderlund
2023-06-20 13:15           ` humin (Q)
2023-06-20 14:10             ` Ferruh Yigit
2023-06-20 14:19               ` Thomas Monjalon
2023-06-21  4:00               ` humin (Q)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).