diff mbox series

examples/kni: fix crash during MTU set

Message ID 20200521151042.3730374-1-ferruh.yigit@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series examples/kni: fix crash during MTU set |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing fail Testing issues

Commit Message

Ferruh Yigit May 21, 2020, 3:10 p.m. UTC 
  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(-)

Comments

Thomas Monjalon May 21, 2020, 3:37 p.m. UTC | #1 
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?
Ferruh Yigit May 21, 2020, 3:46 p.m. UTC | #2 
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.
Rasesh Mody May 21, 2020, 5:32 p.m. UTC | #3 
>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>
Thomas Monjalon May 21, 2020, 8:29 p.m. UTC | #4 
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 :-)
Zhang, XiX May 22, 2020, 2:17 a.m. UTC | #5 
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
Thomas Monjalon May 24, 2020, 3:50 p.m. UTC | #6 
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
diff mbox series

Patch

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)