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 <echau...@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);
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
