[ovs-dev] [PATCH v5] netdev-dpdk: add support for the RTE_ETH_EVENT_INTR_RESET event
Eelco Chaudron
echaudro at redhat.com
Mon Nov 11 14:01:26 UTC 2019
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 at 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);
>>
More information about the dev
mailing list