[ovs-dev,v2] netdev-dpdk: add support for the RTE_ETH_EVENT_INTR_RESET event

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

Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
v1 -> v2:
  - Fixed Ilya's comments

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

Bleep bloop.  Greetings Eelco Chaudron, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#66 FILE: lib/netdev-dpdk.c:1756:
            VLOG_DBG_RL(&rl, "Port "DPDK_PORT_ID_FMT" received a reset request",

Lines checked: 132, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

On 10.09.2019 15:12, 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
> 
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---
> v1 -> v2:
>   - Fixed Ilya's comments
> 
>  lib/netdev-dpdk.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 4805783..37a431e 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -396,6 +396,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 +1175,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;

Thanks.

> +    dev->reset_needed = false;
>  
>      ovsrcu_init(&dev->qos_conf, NULL);
>  
> @@ -1769,6 +1773,34 @@ 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, "Port "DPDK_PORT_ID_FMT" received a reset request",
> +                        dev->port_id);

It'll be better to use a netdev name here. This will also allow to avoid checkpatch
warning.  Something like this:

               VLOG_DBG_RL(&rl, "%s: Device reset requested.",
                           netdev_get_name(&dev->up));

> +            ovs_mutex_unlock(&dev->mutex);
> +        }
> +        ovs_mutex_unlock(&dpdk_mutex);
> +        break;
> +
> +    default:
> +        /* Ignore all other types */

A period is needed at the end of the comment.

> +        break;
> +   }
> +   return 0;
> +}
> +
>  static void
>  dpdk_set_rxq_config(struct netdev_dpdk *dev, const struct smap *args)
>      OVS_REQUIRES(dev->mutex)
> @@ -3807,6 +3839,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 +3854,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 +4223,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);
> 
> 
>

On 10 Sep 2019, at 16:02, Ilya Maximets wrote:

> On 10.09.2019 15:12, 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
>>
>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>> v1 -> v2:
>>   - Fixed Ilya's comments
>>
>>  lib/netdev-dpdk.c |   53 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 51 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 4805783..37a431e 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -396,6 +396,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 +1175,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;
>
> Thanks.
>
>> +    dev->reset_needed = false;
>>
>>      ovsrcu_init(&dev->qos_conf, NULL);
>>
>> @@ -1769,6 +1773,34 @@ 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, "Port "DPDK_PORT_ID_FMT" received a 
>> reset request",
>> +                        dev->port_id);
>
> It'll be better to use a netdev name here. This will also allow to 
> avoid checkpatch
> warning.  Something like this:
>
>                VLOG_DBG_RL(&rl, "%s: Device reset requested.",
>                            netdev_get_name(&dev->up));
>
>> +            ovs_mutex_unlock(&dev->mutex);
>> +        }
>> +        ovs_mutex_unlock(&dpdk_mutex);
>> +        break;
>> +
>> +    default:
>> +        /* Ignore all other types */
>
> A period is needed at the end of the comment.

Sent out a v3 fixing the two above comments

>> +        break;
>> +   }
>> +   return 0;
>> +}
>> +
>>  static void
>>  dpdk_set_rxq_config(struct netdev_dpdk *dev, const struct smap 
>> *args)
>>      OVS_REQUIRES(dev->mutex)
>> @@ -3807,6 +3839,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 +3854,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 +4223,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);
>>
>>
>>