examples/kni: fix crash during MTU set
Checks
Commit Message
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
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?
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.
>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>
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 :-)
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
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
@@ -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)