[dpdk-dev] [PATCH] examples/kni: fix crash during MTU set

[dpdk-dev] [PATCH] examples/kni: fix crash during MTU set

[Ferruh Yigit]

During MTU set (kni_change_mtu) sample application setup queues, which
can free and re-allocate queues.
Meanwhile sample application keeps continues in Rx/Tx burst calls in
different threads, which may cause crash during queue setup.

Pausing application Rx/Tx calls before MTU set and starts it back
afterwards.

Bugzilla ID: 482
Fixes: a26b116749a3 ("examples/kni: fix MTU change to setup Tx queue")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Rasesh Mody <rmody@marvell.com>
Cc: Xi Zhang <xix.zhang@intel.com>
---
 examples/kni/main.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/examples/kni/main.c b/examples/kni/main.c
index 7a927a50c0..80dd0353e7 100644
--- a/examples/kni/main.c
+++ b/examples/kni/main.c
@@ -768,9 +768,8 @@ monitor_all_ports_link_status(void *arg)
 	return NULL;
 }
 
-/* Callback for request of changing MTU */
 static int
-kni_change_mtu(uint16_t port_id, unsigned int new_mtu)
+kni_change_mtu_(uint16_t port_id, unsigned int new_mtu)
 {
 	int ret;
 	uint16_t nb_rxd = NB_RXD;
@@ -851,6 +850,19 @@ kni_change_mtu(uint16_t port_id, unsigned int new_mtu)
 	return 0;
 }
 
+/* Callback for request of changing MTU */
+static int
+kni_change_mtu(uint16_t port_id, unsigned int new_mtu)
+{
+	int ret;
+
+	rte_atomic32_inc(&kni_pause);
+	ret =  kni_change_mtu_(port_id, new_mtu);
+	rte_atomic32_dec(&kni_pause);
+
+	return ret;
+}
+
 /* Callback for request of configuring network interface up/down */
 static int
 kni_config_network_interface(uint16_t port_id, uint8_t if_up)
-- 
2.25.4

Re: [dpdk-dev] [PATCH] examples/kni: fix crash during MTU set

[Thomas Monjalon]

21/05/2020 17:10, Ferruh Yigit:
> During MTU set (kni_change_mtu) sample application setup queues, which
> can free and re-allocate queues.
> Meanwhile sample application keeps continues in Rx/Tx burst calls in
> different threads, which may cause crash during queue setup.
> 
> Pausing application Rx/Tx calls before MTU set and starts it back
> afterwards.
> 
> Bugzilla ID: 482
> Fixes: a26b116749a3 ("examples/kni: fix MTU change to setup Tx queue")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> -/* Callback for request of changing MTU */
>  static int
> -kni_change_mtu(uint16_t port_id, unsigned int new_mtu)
> +kni_change_mtu_(uint16_t port_id, unsigned int new_mtu)
[...]
> +/* Callback for request of changing MTU */
> +static int
> +kni_change_mtu(uint16_t port_id, unsigned int new_mtu)
> +{
> +	int ret;
> +
> +	rte_atomic32_inc(&kni_pause);
> +	ret =  kni_change_mtu_(port_id, new_mtu);
> +	rte_atomic32_dec(&kni_pause);
> +
> +	return ret;
> +}

Why creating a new function which is called only once?

Re: [dpdk-dev] [PATCH] examples/kni: fix crash during MTU set

[Ferruh Yigit]

On 5/21/2020 4:37 PM, Thomas Monjalon wrote:
> 21/05/2020 17:10, Ferruh Yigit:
>> During MTU set (kni_change_mtu) sample application setup queues, which
>> can free and re-allocate queues.
>> Meanwhile sample application keeps continues in Rx/Tx burst calls in
>> different threads, which may cause crash during queue setup.
>>
>> Pausing application Rx/Tx calls before MTU set and starts it back
>> afterwards.
>>
>> Bugzilla ID: 482
>> Fixes: a26b116749a3 ("examples/kni: fix MTU change to setup Tx queue")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> -/* Callback for request of changing MTU */
>>  static int
>> -kni_change_mtu(uint16_t port_id, unsigned int new_mtu)
>> +kni_change_mtu_(uint16_t port_id, unsigned int new_mtu)
> [...]
>> +/* Callback for request of changing MTU */
>> +static int
>> +kni_change_mtu(uint16_t port_id, unsigned int new_mtu)
>> +{
>> +	int ret;
>> +
>> +	rte_atomic32_inc(&kni_pause);
>> +	ret =  kni_change_mtu_(port_id, new_mtu);
>> +	rte_atomic32_dec(&kni_pause);
>> +
>> +	return ret;
>> +}
> 
> Why creating a new function which is called only once?
> 

Just wrapping the existing one with stop/start the forwarding.

These can be added into the existing function but there are many exit points in
the function, so it will create more clutter and it is more error prone. I think
this way more simple and clear.

Re: [dpdk-dev] [EXT] [PATCH] examples/kni: fix crash during MTU set

[Rasesh Mody]

>From: Ferruh Yigit <ferruh.yigit@intel.com>
>Sent: Thursday, May 21, 2020 8:11 AM
>
>External Email
>
>----------------------------------------------------------------------
>During MTU set (kni_change_mtu) sample application setup queues, which
>can free and re-allocate queues.
>Meanwhile sample application keeps continues in Rx/Tx burst calls in different
>threads, which may cause crash during queue setup.
>
>Pausing application Rx/Tx calls before MTU set and starts it back afterwards.
>
>Bugzilla ID: 482
>Fixes: a26b116749a3 ("examples/kni: fix MTU change to setup Tx queue")
>Cc: stable@dpdk.org
>
>Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>---

Acked-by: Rasesh Mody <rmody@marvell.com>

Re: [dpdk-dev] [PATCH] examples/kni: fix crash during MTU set

[Thomas Monjalon]

21/05/2020 17:46, Ferruh Yigit:
> On 5/21/2020 4:37 PM, Thomas Monjalon wrote:
> > 21/05/2020 17:10, Ferruh Yigit:
> >> During MTU set (kni_change_mtu) sample application setup queues, which
> >> can free and re-allocate queues.
> >> Meanwhile sample application keeps continues in Rx/Tx burst calls in
> >> different threads, which may cause crash during queue setup.
> >>
> >> Pausing application Rx/Tx calls before MTU set and starts it back
> >> afterwards.
> >>
> >> Bugzilla ID: 482
> >> Fixes: a26b116749a3 ("examples/kni: fix MTU change to setup Tx queue")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >> ---
> >> -/* Callback for request of changing MTU */
> >>  static int
> >> -kni_change_mtu(uint16_t port_id, unsigned int new_mtu)
> >> +kni_change_mtu_(uint16_t port_id, unsigned int new_mtu)
> > [...]
> >> +/* Callback for request of changing MTU */
> >> +static int
> >> +kni_change_mtu(uint16_t port_id, unsigned int new_mtu)
> >> +{
> >> +	int ret;
> >> +
> >> +	rte_atomic32_inc(&kni_pause);
> >> +	ret =  kni_change_mtu_(port_id, new_mtu);
> >> +	rte_atomic32_dec(&kni_pause);
> >> +
> >> +	return ret;
> >> +}
> > 
> > Why creating a new function which is called only once?
> > 
> 
> Just wrapping the existing one with stop/start the forwarding.
> 
> These can be added into the existing function but there are many exit points in
> the function, so it will create more clutter and it is more error prone. I think
> this way more simple and clear.

OK to go on the safe side at this stage of the release :-)

Re: [dpdk-dev] [PATCH] examples/kni: fix crash during MTU set

[Zhang, XiX]

Tested-by:zhang,xi <xix.zhang@intel.com>

-----Original Message-----
From: Yigit, Ferruh 
Sent: Thursday, May 21, 2020 11:11 PM
To: Rasesh Mody <rmody@marvell.com>
Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; stable@dpdk.org; Zhang, XiX <xix.zhang@intel.com>
Subject: [PATCH] examples/kni: fix crash during MTU set

During MTU set (kni_change_mtu) sample application setup queues, which can free and re-allocate queues.
Meanwhile sample application keeps continues in Rx/Tx burst calls in different threads, which may cause crash during queue setup.

Pausing application Rx/Tx calls before MTU set and starts it back afterwards.

Bugzilla ID: 482
Fixes: a26b116749a3 ("examples/kni: fix MTU change to setup Tx queue")
Cc: stable@dpdk.org

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Rasesh Mody <rmody@marvell.com>
Cc: Xi Zhang <xix.zhang@intel.com>
---
 examples/kni/main.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/examples/kni/main.c b/examples/kni/main.c index 7a927a50c0..80dd0353e7 100644
--- a/examples/kni/main.c
+++ b/examples/kni/main.c
@@ -768,9 +768,8 @@ monitor_all_ports_link_status(void *arg)
 	return NULL;
 }
 
-/* Callback for request of changing MTU */  static int -kni_change_mtu(uint16_t port_id, unsigned int new_mtu)
+kni_change_mtu_(uint16_t port_id, unsigned int new_mtu)
 {
 	int ret;
 	uint16_t nb_rxd = NB_RXD;
@@ -851,6 +850,19 @@ kni_change_mtu(uint16_t port_id, unsigned int new_mtu)
 	return 0;
 }
 
+/* Callback for request of changing MTU */ static int 
+kni_change_mtu(uint16_t port_id, unsigned int new_mtu) {
+	int ret;
+
+	rte_atomic32_inc(&kni_pause);
+	ret =  kni_change_mtu_(port_id, new_mtu);
+	rte_atomic32_dec(&kni_pause);
+
+	return ret;
+}
+
 /* Callback for request of configuring network interface up/down */  static int  kni_config_network_interface(uint16_t port_id, uint8_t if_up)
--
2.25.4

Re: [dpdk-dev] [EXT] [PATCH] examples/kni: fix crash during MTU set

[Thomas Monjalon]

21/05/2020 19:32, Rasesh Mody:
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >During MTU set (kni_change_mtu) sample application setup queues, which
> >can free and re-allocate queues.
> >Meanwhile sample application keeps continues in Rx/Tx burst calls in different
> >threads, which may cause crash during queue setup.
> >
> >Pausing application Rx/Tx calls before MTU set and starts it back afterwards.
> >
> >Bugzilla ID: 482
> >Fixes: a26b116749a3 ("examples/kni: fix MTU change to setup Tx queue")
> >Cc: stable@dpdk.org
> >
> >Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Acked-by: Rasesh Mody <rmody@marvell.com>

Applied, thanks