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

[Ilya Maximets]

On 05.09.2019 17:44, Eelco Chaudron wrote:
> 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) {
> 

OK. I see.
Please, add a space between cast and variable: 'switch ((int) 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);
>>>
> 
>