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

[Eelco Chaudron]

See inlines below, and will sent a v2 early next week.

On 5 Sep 2019, at 14:40, Ilya Maximets wrote:

> Hi Eelco,
> Thanks for the patch! Looks reasonable.
>
> One comment is that it's better to explicitly initialize the
> flag in common_construct.  I see that we doesn't initialize
> 'started' flag, but this might be fixed too.
>
> More comments inline.
>
> Best regards, Ilya Maximets.
>
> On 05.09.2019 14:48, 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 at redhat.com>
>> ---
>>  lib/netdev-dpdk.c | 49 
>> +++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index bc20d6843..a23150387 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -362,6 +362,7 @@ struct netdev_dpdk {
>>          bool attached;
>>          /* If true, rte_eth_dev_start() was successfully called */
>>          bool started;
>> +        bool reset_needed;
>
> This will produce a hole in the structure as members will no longer
> fit into a single cacheline.  And cacheline markers will be 
> misaligned.
>
> See details for similar issue here:
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-September/362349.html
>
> Possible solution is to turn existing flags into bit fields or stop
> avoiding the problem and drop most of padding in the structure.

Will change this…

>>          struct eth_addr hwaddr;
>>          int mtu;
>>          int socket_id;
>> @@ -1735,6 +1736,34 @@ netdev_dpdk_process_devargs(struct netdev_dpdk 
>> *dev,
>>      return new_port_id;
>>  }
>>
>> +static int
>> +dpdk_eth_event_callback(uint16_t port_id, enum rte_eth_event_type 
>> type,
>
> s/uint16_t/dpdk_port_t/
>
>> +                        void *param OVS_UNUSED, void *ret_param 
>> OVS_UNUSED)
>> +{
>> +    struct netdev_dpdk *dev;
>> +
>> +    switch ((int)type) {
>
> Do we need this cast?

In my environment this flag is enabled, not sure if I should change this 
some where else…

lib/netdev-dpdk.c:1747:5: error: enumeration value 
'RTE_ETH_EVENT_UNKNOWN' not handled in switch [-Werror=switch-enum]
      switch (type) {

>> +
>
> Empty line not needed.
>
ack
>> +    case RTE_ETH_EVENT_INTR_RESET:
>> +
>
> Empty line not needed.
>
ack
>> +        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);> +            
>> ovs_mutex_unlock(&dev->mutex);
>
> Maybe it'll be good to add at leas debug logs here?
Will ad a log message
>> +        }
>> +        ovs_mutex_unlock(&dpdk_mutex);
>> +        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)
>> @@ -3777,6 +3806,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,
>> @@ -3790,6 +3821,14 @@ 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 %d", 
>> ret);
>
> It's better to print rte_strerror(-ret).
ack
>> +        }
>> +
>>          ovsthread_once_done(&once);
>>      }
>>
>> @@ -4150,13 +4189,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);
>>