[ovs-dev,v5] netdev-dpdk: add support for the RTE_ETH_EVENT_INTR_RESET event
diff mbox series

Message ID 20191106134245.165674.92584.stgit@netdev64
State Changes Requested
Headers show
Series
  • [ovs-dev,v5] netdev-dpdk: add support for the RTE_ETH_EVENT_INTR_RESET event
Related show

Commit Message

Eelco Chaudron Nov. 6, 2019, 1:43 p.m. UTC
Currently, OVS does not register and therefore not handle the
interface reset event from the DPDK framework. This would cause a
problem in cases where a VF is used as an interface, and its
configuration changes.

As an example in the following scenario the MAC change is not
detected/acted upon until OVS is restarted without the patch applied:

  $ echo 1 > /sys/bus/pci/devices/0000:05:00.1/sriov_numvfs
  $ ovs-vsctl add-port ovs_pvp_br0 dpdk0 -- \
            set Interface dpdk0 type=dpdk -- \
            set Interface dpdk0 options:dpdk-devargs=0000:05:0a.0

  $ ip link set p5p2 vf 0 mac 52:54:00:92:d3:33

Requires patch "bridge: Allow manual notifications about interfaces' updates."

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
v4 -> v5:
  Using new if_notifier_manual_report() API

v3 -> v4:
  Add support for if-notification to make sure set_config()
  gets called

v2 -> v3:
v1 -> v2:
  Fixed Ilya's comments

lib/netdev-dpdk.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 2 deletions(-)

Comments

Ilya Maximets Nov. 7, 2019, 11:50 a.m. UTC | #1
On 06.11.2019 14:43, Eelco Chaudron wrote:
> Currently, OVS does not register and therefore not handle the
> interface reset event from the DPDK framework. This would cause a
> problem in cases where a VF is used as an interface, and its
> configuration changes.
> 
> As an example in the following scenario the MAC change is not
> detected/acted upon until OVS is restarted without the patch applied:
> 
>    $ echo 1 > /sys/bus/pci/devices/0000:05:00.1/sriov_numvfs
>    $ ovs-vsctl add-port ovs_pvp_br0 dpdk0 -- \
>              set Interface dpdk0 type=dpdk -- \
>              set Interface dpdk0 options:dpdk-devargs=0000:05:0a.0
> 
>    $ ip link set p5p2 vf 0 mac 52:54:00:92:d3:33
> 
> Requires patch "bridge: Allow manual notifications about interfaces' updates."
> 
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---
> v4 -> v5:
>    Using new if_notifier_manual_report() API
> 
> v3 -> v4:
>    Add support for if-notification to make sure set_config()
>    gets called
> 
> v2 -> v3:
> v1 -> v2:
>    Fixed Ilya's comments
> 
> lib/netdev-dpdk.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 4805783..b744589 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -46,6 +46,7 @@
>   #include "dpdk.h"
>   #include "dpif-netdev.h"
>   #include "fatal-signal.h"
> +#include "if-notifier.h"
>   #include "netdev-provider.h"
>   #include "netdev-vport.h"
>   #include "odp-util.h"
> @@ -396,6 +397,8 @@ struct netdev_dpdk {
>           bool attached;
>           /* If true, rte_eth_dev_start() was successfully called */
>           bool started;
> +        bool reset_needed;
> +        /* 1 pad byte here. */
>           struct eth_addr hwaddr;
>           int mtu;
>           int socket_id;
> @@ -1173,6 +1176,8 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no,
>       ovsrcu_index_init(&dev->vid, -1);
>       dev->vhost_reconfigured = false;
>       dev->attached = false;
> +    dev->started = false;
> +    dev->reset_needed = false;
>   
>       ovsrcu_init(&dev->qos_conf, NULL);
>   
> @@ -1769,6 +1774,36 @@ netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
>       return new_port_id;
>   }
>   
> +static int
> +dpdk_eth_event_callback(dpdk_port_t port_id, enum rte_eth_event_type type,
> +                        void *param OVS_UNUSED, void *ret_param OVS_UNUSED)
> +{
> +    struct netdev_dpdk *dev;
> +
> +    switch ((int) type) {
> +    case RTE_ETH_EVENT_INTR_RESET:
> +        ovs_mutex_lock(&dpdk_mutex);
> +        dev = netdev_dpdk_lookup_by_port_id(port_id);
> +        if (dev) {
> +            ovs_mutex_lock(&dev->mutex);
> +            dev->reset_needed = true;
> +            netdev_request_reconfigure(&dev->up);
> +            VLOG_DBG_RL(&rl, "%s: Device reset requested.",
> +                        netdev_get_name(&dev->up));
> +            ovs_mutex_unlock(&dev->mutex);
> +        }
> +        ovs_mutex_unlock(&dpdk_mutex);
> +
> +        if_notifier_manual_report();
> +        break;
> +
> +    default:
> +        /* Ignore all other types. */
> +        break;
> +   }
> +   return 0;
> +}
> +
>   static void
>   dpdk_set_rxq_config(struct netdev_dpdk *dev, const struct smap *args)
>       OVS_REQUIRES(dev->mutex)
> @@ -3807,6 +3842,8 @@ netdev_dpdk_class_init(void)
>       /* This function can be called for different classes.  The initialization
>        * needs to be done only once */
>       if (ovsthread_once_start(&once)) {
> +        int ret;
> +
>           ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL);
>           unixctl_command_register("netdev-dpdk/set-admin-state",
>                                    "[netdev] up|down", 1, 2,
> @@ -3820,6 +3857,15 @@ netdev_dpdk_class_init(void)
>                                    "[netdev]", 0, 1,
>                                    netdev_dpdk_get_mempool_info, NULL);
>   
> +        ret = rte_eth_dev_callback_register(RTE_ETH_ALL,
> +                                            RTE_ETH_EVENT_INTR_RESET,
> +                                            dpdk_eth_event_callback, NULL);
> +

This empty line could be removed.  There is already enough empty space.

> +        if (ret != 0) {
> +            VLOG_ERR("Ethernet device callback register error: %s",
> +                     rte_strerror(-ret));
> +        }
> +
>           ovsthread_once_done(&once);
>       }
>   
> @@ -4180,13 +4226,19 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>           && dev->rxq_size == dev->requested_rxq_size
>           && dev->txq_size == dev->requested_txq_size
>           && dev->socket_id == dev->requested_socket_id
> -        && dev->started) {
> +        && dev->started && !dev->reset_needed) {
>           /* Reconfiguration is unnecessary */
>   
>           goto out;
>       }
>   
> -    rte_eth_dev_stop(dev->port_id);
> +    if (dev->reset_needed) {
> +        rte_eth_dev_reset(dev->port_id);

Thinking more about the configuration sequence it seems that call
if_notifier_manual_report() should be here and not in the callback
to be sure that settings will be applied after reset.

BTW, even if this will be always called from the main thread, I
still think that notifier itself should be thread-safe as we could
use it later directly from the callback for some other event types.

> +        dev->reset_needed = false;
> +    } else {
> +        rte_eth_dev_stop(dev->port_id);
> +    }
> +
>       dev->started = false;
>   
>       err = netdev_dpdk_mempool_configure(dev);
>
Eelco Chaudron Nov. 11, 2019, 2:01 p.m. UTC | #2
On 7 Nov 2019, at 12:50, Ilya Maximets wrote:

> On 06.11.2019 14:43, Eelco Chaudron wrote:
>> Currently, OVS does not register and therefore not handle the
>> interface reset event from the DPDK framework. This would cause a
>> problem in cases where a VF is used as an interface, and its
>> configuration changes.
>>
>> As an example in the following scenario the MAC change is not
>> detected/acted upon until OVS is restarted without the patch applied:
>>
>>    $ echo 1 > /sys/bus/pci/devices/0000:05:00.1/sriov_numvfs
>>    $ ovs-vsctl add-port ovs_pvp_br0 dpdk0 -- \
>>              set Interface dpdk0 type=dpdk -- \
>>              set Interface dpdk0 options:dpdk-devargs=0000:05:0a.0
>>
>>    $ ip link set p5p2 vf 0 mac 52:54:00:92:d3:33
>>
>> Requires patch "bridge: Allow manual notifications about interfaces' 
>> updates."
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>> v4 -> v5:
>>    Using new if_notifier_manual_report() API
>>
>> v3 -> v4:
>>    Add support for if-notification to make sure set_config()
>>    gets called
>>
>> v2 -> v3:
>> v1 -> v2:
>>    Fixed Ilya's comments
>>
>> lib/netdev-dpdk.c |   56 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 4805783..b744589 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -46,6 +46,7 @@
>>   #include "dpdk.h"
>>   #include "dpif-netdev.h"
>>   #include "fatal-signal.h"
>> +#include "if-notifier.h"
>>   #include "netdev-provider.h"
>>   #include "netdev-vport.h"
>>   #include "odp-util.h"
>> @@ -396,6 +397,8 @@ struct netdev_dpdk {
>>           bool attached;
>>           /* If true, rte_eth_dev_start() was successfully called */
>>           bool started;
>> +        bool reset_needed;
>> +        /* 1 pad byte here. */
>>           struct eth_addr hwaddr;
>>           int mtu;
>>           int socket_id;
>> @@ -1173,6 +1176,8 @@ common_construct(struct netdev *netdev, 
>> dpdk_port_t port_no,
>>       ovsrcu_index_init(&dev->vid, -1);
>>       dev->vhost_reconfigured = false;
>>       dev->attached = false;
>> +    dev->started = false;
>> +    dev->reset_needed = false;
>>        ovsrcu_init(&dev->qos_conf, NULL);
>>  @@ -1769,6 +1774,36 @@ netdev_dpdk_process_devargs(struct 
>> netdev_dpdk *dev,
>>       return new_port_id;
>>   }
>>  +static int
>> +dpdk_eth_event_callback(dpdk_port_t port_id, enum rte_eth_event_type 
>> type,
>> +                        void *param OVS_UNUSED, void *ret_param 
>> OVS_UNUSED)
>> +{
>> +    struct netdev_dpdk *dev;
>> +
>> +    switch ((int) type) {
>> +    case RTE_ETH_EVENT_INTR_RESET:
>> +        ovs_mutex_lock(&dpdk_mutex);
>> +        dev = netdev_dpdk_lookup_by_port_id(port_id);
>> +        if (dev) {
>> +            ovs_mutex_lock(&dev->mutex);
>> +            dev->reset_needed = true;
>> +            netdev_request_reconfigure(&dev->up);
>> +            VLOG_DBG_RL(&rl, "%s: Device reset requested.",
>> +                        netdev_get_name(&dev->up));
>> +            ovs_mutex_unlock(&dev->mutex);
>> +        }
>> +        ovs_mutex_unlock(&dpdk_mutex);
>> +
>> +        if_notifier_manual_report();
>> +        break;
>> +
>> +    default:
>> +        /* Ignore all other types. */
>> +        break;
>> +   }
>> +   return 0;
>> +}
>> +
>>   static void
>>   dpdk_set_rxq_config(struct netdev_dpdk *dev, const struct smap 
>> *args)
>>       OVS_REQUIRES(dev->mutex)
>> @@ -3807,6 +3842,8 @@ netdev_dpdk_class_init(void)
>>       /* This function can be called for different classes.  The 
>> initialization
>>        * needs to be done only once */
>>       if (ovsthread_once_start(&once)) {
>> +        int ret;
>> +
>>           ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL);
>>           unixctl_command_register("netdev-dpdk/set-admin-state",
>>                                    "[netdev] up|down", 1, 2,
>> @@ -3820,6 +3857,15 @@ netdev_dpdk_class_init(void)
>>                                    "[netdev]", 0, 1,
>>                                    netdev_dpdk_get_mempool_info, 
>> NULL);
>>  +        ret = rte_eth_dev_callback_register(RTE_ETH_ALL,
>> +                                            
>> RTE_ETH_EVENT_INTR_RESET,
>> +                                            dpdk_eth_event_callback, 
>> NULL);
>> +
>
> This empty line could be removed.  There is already enough empty 
> space.


Will do…

>> +        if (ret != 0) {
>> +            VLOG_ERR("Ethernet device callback register error: %s",
>> +                     rte_strerror(-ret));
>> +        }
>> +
>>           ovsthread_once_done(&once);
>>       }
>>  @@ -4180,13 +4226,19 @@ netdev_dpdk_reconfigure(struct netdev 
>> *netdev)
>>           && dev->rxq_size == dev->requested_rxq_size
>>           && dev->txq_size == dev->requested_txq_size
>>           && dev->socket_id == dev->requested_socket_id
>> -        && dev->started) {
>> +        && dev->started && !dev->reset_needed) {
>>           /* Reconfiguration is unnecessary */
>>            goto out;
>>       }
>>  -    rte_eth_dev_stop(dev->port_id);
>> +    if (dev->reset_needed) {
>> +        rte_eth_dev_reset(dev->port_id);
>
> Thinking more about the configuration sequence it seems that call
> if_notifier_manual_report() should be here and not in the callback
> to be sure that settings will be applied after reset.

I did verify the order would be correct in the current implementation, 
however this way it makes way more sense (and is more clear).

Will sent out a v6 soon…

> BTW, even if this will be always called from the main thread, I
> still think that notifier itself should be thread-safe as we could
> use it later directly from the callback for some other event types.
>
>> +        dev->reset_needed = false;
>> +    } else {
>> +        rte_eth_dev_stop(dev->port_id);
>> +    }
>> +
>>       dev->started = false;
>>        err = netdev_dpdk_mempool_configure(dev);
>>

Patch
diff mbox series

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 4805783..b744589 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -46,6 +46,7 @@ 
 #include "dpdk.h"
 #include "dpif-netdev.h"
 #include "fatal-signal.h"
+#include "if-notifier.h"
 #include "netdev-provider.h"
 #include "netdev-vport.h"
 #include "odp-util.h"
@@ -396,6 +397,8 @@  struct netdev_dpdk {
         bool attached;
         /* If true, rte_eth_dev_start() was successfully called */
         bool started;
+        bool reset_needed;
+        /* 1 pad byte here. */
         struct eth_addr hwaddr;
         int mtu;
         int socket_id;
@@ -1173,6 +1176,8 @@  common_construct(struct netdev *netdev, dpdk_port_t port_no,
     ovsrcu_index_init(&dev->vid, -1);
     dev->vhost_reconfigured = false;
     dev->attached = false;
+    dev->started = false;
+    dev->reset_needed = false;
 
     ovsrcu_init(&dev->qos_conf, NULL);
 
@@ -1769,6 +1774,36 @@  netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
     return new_port_id;
 }
 
+static int
+dpdk_eth_event_callback(dpdk_port_t port_id, enum rte_eth_event_type type,
+                        void *param OVS_UNUSED, void *ret_param OVS_UNUSED)
+{
+    struct netdev_dpdk *dev;
+
+    switch ((int) type) {
+    case RTE_ETH_EVENT_INTR_RESET:
+        ovs_mutex_lock(&dpdk_mutex);
+        dev = netdev_dpdk_lookup_by_port_id(port_id);
+        if (dev) {
+            ovs_mutex_lock(&dev->mutex);
+            dev->reset_needed = true;
+            netdev_request_reconfigure(&dev->up);
+            VLOG_DBG_RL(&rl, "%s: Device reset requested.",
+                        netdev_get_name(&dev->up));
+            ovs_mutex_unlock(&dev->mutex);
+        }
+        ovs_mutex_unlock(&dpdk_mutex);
+
+        if_notifier_manual_report();
+        break;
+
+    default:
+        /* Ignore all other types. */
+        break;
+   }
+   return 0;
+}
+
 static void
 dpdk_set_rxq_config(struct netdev_dpdk *dev, const struct smap *args)
     OVS_REQUIRES(dev->mutex)
@@ -3807,6 +3842,8 @@  netdev_dpdk_class_init(void)
     /* This function can be called for different classes.  The initialization
      * needs to be done only once */
     if (ovsthread_once_start(&once)) {
+        int ret;
+
         ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL);
         unixctl_command_register("netdev-dpdk/set-admin-state",
                                  "[netdev] up|down", 1, 2,
@@ -3820,6 +3857,15 @@  netdev_dpdk_class_init(void)
                                  "[netdev]", 0, 1,
                                  netdev_dpdk_get_mempool_info, NULL);
 
+        ret = rte_eth_dev_callback_register(RTE_ETH_ALL,
+                                            RTE_ETH_EVENT_INTR_RESET,
+                                            dpdk_eth_event_callback, NULL);
+
+        if (ret != 0) {
+            VLOG_ERR("Ethernet device callback register error: %s",
+                     rte_strerror(-ret));
+        }
+
         ovsthread_once_done(&once);
     }
 
@@ -4180,13 +4226,19 @@  netdev_dpdk_reconfigure(struct netdev *netdev)
         && dev->rxq_size == dev->requested_rxq_size
         && dev->txq_size == dev->requested_txq_size
         && dev->socket_id == dev->requested_socket_id
-        && dev->started) {
+        && dev->started && !dev->reset_needed) {
         /* Reconfiguration is unnecessary */
 
         goto out;
     }
 
-    rte_eth_dev_stop(dev->port_id);
+    if (dev->reset_needed) {
+        rte_eth_dev_reset(dev->port_id);
+        dev->reset_needed = false;
+    } else {
+        rte_eth_dev_stop(dev->port_id);
+    }
+
     dev->started = false;
 
     err = netdev_dpdk_mempool_configure(dev);