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

[Eelco Chaudron]

On 12 Sep 2019, at 12:24, Ilya Maximets wrote:

> On 12.09.2019 13:19, Ilya Maximets wrote:
>> On 12.09.2019 13:07, Eelco Chaudron wrote:
>>>
>>>
>>> On 12 Sep 2019, at 10:39, Ilya Maximets wrote:
>>>
>>>> On 11.09.2019 16:20, 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>
>>>>> ---
>>>>
>>>> <snip>
>>>>
>>>>> @@ -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);
>>>>
>>>> Thinking more about the change and looking at flow control 
>>>> configuration,
>>>> it seems that on reset we'll lost configurations done in 
>>>> set_config().
>>>> Device reset should return it to initial state, i.e. all the 
>>>> default settings,
>>>> but set_config() will not be called after that.
>>>> I know, that VFs commonly doesn't support flow control, but if 
>>>> we'll add like
>>>> real configuration of MAC address, it will be lost too.
>>>>
>>>> What do you think?
>>>
>>> Doesn’t the full bridge run sequence take care of this?
>>>
>>> So in callback we do netdev_request_reconfigure() which triggers the 
>>> following sequence…
>>>
>>> bridge_run__()
>>>  ...
>>>    dpif_netdev_run()
>>>      ...
>>>        reconfigure_datapath()
>>>          ...
>>>            netdev_dpdk_reconfigure()
>>>
>>> But than also it will call in the next run
>>>
>>> bridge_run()
>>>  ...
>>>    bridge_delete_or_reconfigure_ports()
>>>      ...
>>>         netdev_set_config()
>>>           netdev_dpdk_set_config()
>>>
>>>
>>> Or do I miss something?
>>
>> dpif_netdev_run() is called from ofproto_type_run() which is called
>> unconditionally from the bridge_run().
>> But bridge_delete_or_reconfigure_ports() only called from 
>> bridge_reconfigure(),
>> which is protected by the following condition:
>>
>>     if (ovsdb_idl_get_seqno(idl) != idl_seqno ||
>>         if_notifier_changed(ifnotifier)) {
>>
>> i.e. if there will be no DB updates or there will be no netlink 
>> notifications,
>> set_config() will not be called. I understand that in your scenario 
>> there
>> might be netlink notification for interface changes since PF is 
>> controlled
>> by the kernel, but it might be not the case if PF attached as a DPDK 
>> port
>> to OVS or some other application.
>
> Hmm. Basically, dpdk_eth_event_callback() is an analogue of the
> if_notifier, but for DPDK application.
>
> Maybe we can add it as another trigger for above 'if' condition?

It sounds like a good idea, however, I see no clean way of doing this as 
the if_notifier_changed()/if_notifier_create() is not data path 
abstracted.
Any ideas for a nice clean solution here? Guess we could add a 
dpdk_notifier_create() in if-notifier.c

Thoughts?