Re: [ovs-dev] [ovs-dev v2] dpif-netdev: fix dpif_netdev_flow_put
Bleep bloop. Greetings Peng He, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author Peng He needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Peng He Lines checked: 199, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [ovs-dev v2] dpif-netdev: fix dpif_netdev_flow_put
OVS allows overlapping megaflows, as long as the actions of these
megaflows are equal. However, the current implementation of action
modification relies on flow_lookup instead of ufid, this could result
in looking up a wrong megaflow and make the ukeys and megaflows inconsistent
Just like the test case in the patch, at first we have a rule with the
prefix:
10.1.2.0/24
and we will get a megaflow with prefixes 10.1.2.2/24 when a packet with IP
10.1.2.2 is received.
Then suppose we change the rule into 10.1.0.0/16. OVS prefers to keep the
10.1.2.2/24 megaflow and just changes its action instead of extending
the prefix into 10.1.2.2/16.
then suppose we have a 10.1.0.2 packet, since it misses the megaflow,
this time, we will have an overlapping megaflow with the right prefix:
10.1.0.2/16
now we have two megaflows:
10.1.2.2/24
10.1.0.2/16
last, suppose we have changed the ruleset again. The revalidator this
time still decides to change the actions of both megaflows instead of
deleting them.
The dpif_netdev_flow_put will search the megaflow to modify with unmasked
keys, however it might lookup the wrong megaflow as the key 10.1.2.2 matches
both 10.1.2.2/24 and 10.1.0.2/16!
This patch changes the megaflow lookup code in modification path into
relying the ufid to find the correct megaflow instead of key lookup.
Signed-off-by: Peng He
---
lib/dpif-netdev.c | 62 ++-
tests/pmd.at | 46 +++
2 files changed, 80 insertions(+), 28 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 70b953ae6..b90ed1542 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4198,36 +4198,43 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd,
memset(stats, 0, sizeof *stats);
}
-ovs_mutex_lock(&pmd->flow_mutex);
-netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL);
-if (!netdev_flow) {
-if (put->flags & DPIF_FP_CREATE) {
+if (put->flags & DPIF_FP_CREATE) {
+ovs_mutex_lock(&pmd->flow_mutex);
+netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL);
+if (!netdev_flow) {
dp_netdev_flow_add(pmd, match, ufid, put->actions,
put->actions_len, ODPP_NONE);
} else {
-error = ENOENT;
+error = EEXIST;
}
+ovs_mutex_unlock(&pmd->flow_mutex);
} else {
+netdev_flow = dp_netdev_pmd_find_flow(pmd, ufid,
+ put->key, put->key_len);
+
if (put->flags & DPIF_FP_MODIFY) {
-struct dp_netdev_actions *new_actions;
-struct dp_netdev_actions *old_actions;
+if (!netdev_flow) {
+error = ENOENT;
+} else {
+struct dp_netdev_actions *new_actions;
+struct dp_netdev_actions *old_actions;
-new_actions = dp_netdev_actions_create(put->actions,
- put->actions_len);
+new_actions = dp_netdev_actions_create(put->actions,
+ put->actions_len);
-old_actions = dp_netdev_flow_get_actions(netdev_flow);
-ovsrcu_set(&netdev_flow->actions, new_actions);
+old_actions = dp_netdev_flow_get_actions(netdev_flow);
+ovsrcu_set(&netdev_flow->actions, new_actions);
-queue_netdev_flow_put(pmd, netdev_flow, match,
- put->actions, put->actions_len,
- DP_NETDEV_FLOW_OFFLOAD_OP_MOD);
-log_netdev_flow_change(netdev_flow, match, old_actions,
- put->actions, put->actions_len);
+queue_netdev_flow_put(pmd, netdev_flow, match,
+ put->actions, put->actions_len,
+ DP_NETDEV_FLOW_OFFLOAD_OP_MOD);
+log_netdev_flow_change(netdev_flow, match, old_actions,
+ put->actions, put->actions_len);
-if (stats) {
-get_dpif_flow_status(pmd->dp, netdev_flow, stats, NULL);
-}
-if (put->flags & DPIF_FP_ZERO_STATS) {
+if (stats) {
+get_dpif_flow_status(pmd->dp, netdev_flow, stats, NULL);
+}
+if (put->flags & DPIF_FP_ZERO_STATS) {
/* XXX: The userspace datapath uses thread local statistics
* (for flows), which should be updated only by the owning
* thread. Since we cannot write on stats memory here,
@@ -4237,18 +4244,17 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd,
* - Should the need arise, this operation can be implemented
* by keeping a base value (to be update here) for each
* counter, and subtracting it
[ovs-dev] [PATCH V3 2/2] dpif-netdev: Fix flushing of a vport
When using a userspace vport ("vxlan0"), dpif-netdev adds an additional
netdev ("vxlan_sys_4789"). The dpif netdev ("vxlan0") is added to the
netdev-offload ports map, thus flows are associated on this netdev.
However, flushing is done on the dpif-netdev level ("vxlan_sys_4789"),
and relevant offload flows are not destroyed.
To fix it, add the datapath netdev to the netdev-offload ports map. In
case there is no different internal netdev, use the dpif netdev, as before.
Fixes: adbd4301a249 ("netdev-offload-dpdk: Use per-netdev offload metadata.")
Signed-off-by: Eli Britstein
---
lib/dpif-netdev.c | 15 ++-
lib/dpif-netlink.c | 5 -
lib/dpif-provider.h | 5 +++--
lib/dpif.c | 8 +---
4 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 70b953ae6..52d2998d7 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -547,7 +547,8 @@ static int get_port_by_name(struct dp_netdev *dp, const
char *devname,
static void dp_netdev_free(struct dp_netdev *)
OVS_REQUIRES(dp_netdev_mutex);
static int do_add_port(struct dp_netdev *dp, const char *devname,
- const char *type, odp_port_t port_no)
+ const char *type, odp_port_t port_no,
+ struct netdev **datapath_netdev)
OVS_REQ_WRLOCK(dp->port_rwlock);
static void do_del_port(struct dp_netdev *dp, struct dp_netdev_port *)
OVS_REQ_WRLOCK(dp->port_rwlock);
@@ -1884,7 +1885,7 @@ create_dp_netdev(const char *name, const struct
dpif_class *class,
error = do_add_port(dp, name, dpif_netdev_port_open_type(dp->class,
"internal"),
-ODPP_LOCAL);
+ODPP_LOCAL, NULL);
ovs_rwlock_unlock(&dp->port_rwlock);
if (error) {
dp_netdev_free(dp);
@@ -2151,7 +2152,7 @@ out:
static int
do_add_port(struct dp_netdev *dp, const char *devname, const char *type,
-odp_port_t port_no)
+odp_port_t port_no, struct netdev **datapath_netdev)
OVS_REQ_WRLOCK(dp->port_rwlock)
{
struct netdev_saved_flags *sf;
@@ -2167,6 +2168,9 @@ do_add_port(struct dp_netdev *dp, const char *devname,
const char *type,
if (error) {
return error;
}
+if (datapath_netdev) {
+*datapath_netdev = port->netdev;
+}
hmap_insert(&dp->ports, &port->node, hash_port_no(port_no));
seq_change(dp->port_seq);
@@ -2196,7 +2200,7 @@ do_add_port(struct dp_netdev *dp, const char *devname,
const char *type,
static int
dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev,
- odp_port_t *port_nop)
+ odp_port_t *port_nop, struct netdev **datapath_netdev)
{
struct dp_netdev *dp = get_dp_netdev(dpif);
char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
@@ -2215,7 +2219,8 @@ dpif_netdev_port_add(struct dpif *dpif, struct netdev
*netdev,
}
if (!error) {
*port_nop = port_no;
-error = do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no);
+error = do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no,
+datapath_netdev);
}
ovs_rwlock_unlock(&dp->port_rwlock);
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 60bd39643..a02f0f2d9 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -1144,7 +1144,7 @@ dpif_netlink_rtnl_port_create_and_add(struct dpif_netlink
*dpif,
static int
dpif_netlink_port_add(struct dpif *dpif_, struct netdev *netdev,
- odp_port_t *port_nop)
+ odp_port_t *port_nop, struct netdev **datapath_netdev)
{
struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
int error = EOPNOTSUPP;
@@ -1157,6 +1157,9 @@ dpif_netlink_port_add(struct dpif *dpif_, struct netdev
*netdev,
error = dpif_netlink_port_add_compat(dpif, netdev, port_nop);
}
fat_rwlock_unlock(&dpif->upcall_lock);
+if (datapath_netdev) {
+*datapath_netdev = netdev;
+}
return error;
}
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index a33c6ec30..47c573d95 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -203,10 +203,11 @@ struct dpif_class {
* ODPP_NONE, attempts to use that as the port's port number.
*
* If port is successfully added, sets '*port_no' to the new port's
- * port number. Returns EBUSY if caller attempted to choose a port
+ * port number, and datapath_netdev to a potentially created netdev in the
+ * dpif-class level. Returns EBUSY if caller attempted to choose a port
* number, and it was in use. */
int (*port_add)(struct dpif *dpif, struct netdev *netdev,
-odp_port_t *port_no);
+odp_port_t *port_no, struct netdev **datapath_netdev);
/* Removes port numbered 'port_no' from 'dpif'. */
int (*port_del)(struct dpif *dp
[ovs-dev] [PATCH V3 1/2] netdev-offload-dpdk: Fix flushing of a physdev
Vport's offloads are done on the tracked orig-in-port, but the flow itself
is associated in the vport's map.
Removing the physdev will flush all the ports that are on its map, but
not the ones on other netdevs' maps. Since flows take reference count on
both their vport and their physdev, the physdev still has references on.
Trying to remove it and re-add it fails with "already in use" error.
Fix it by flushing the physdev's offload flows in all related netdevs,
e.g. the netdev itself, or for physical devices, all vports.
Fixes: adbd4301a249 ("netdev-offload-dpdk: Use per-netdev offload metadata.")
Reported-by: 15895987278
Signed-off-by: Eli Britstein
---
lib/netdev-offload-dpdk.c | 35 ++-
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 14bc87771..992627fa2 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -2537,15 +2537,15 @@ out:
return ret;
}
-static int
-netdev_offload_dpdk_flow_flush(struct netdev *netdev)
+static void
+flush_netdev_flows_in_related(struct netdev *netdev, struct netdev *related)
{
-struct cmap *map = offload_data_map(netdev);
-struct ufid_to_rte_flow_data *data;
unsigned int tid = netdev_offload_thread_id();
+struct cmap *map = offload_data_map(related);
+struct ufid_to_rte_flow_data *data;
if (!map) {
-return -1;
+return;
}
CMAP_FOR_EACH (data, node, map) {
@@ -2556,6 +2556,31 @@ netdev_offload_dpdk_flow_flush(struct netdev *netdev)
netdev_offload_dpdk_flow_destroy(data);
}
}
+}
+
+static bool
+flush_in_vport_cb(struct netdev *vport,
+ odp_port_t odp_port OVS_UNUSED,
+ void *aux)
+{
+struct netdev *netdev = aux;
+
+/* Only vports are related to physical devices. */
+if (netdev_vport_is_vport_class(vport->netdev_class)) {
+flush_netdev_flows_in_related(netdev, vport);
+}
+
+return false;
+}
+
+static int
+netdev_offload_dpdk_flow_flush(struct netdev *netdev)
+{
+flush_netdev_flows_in_related(netdev, netdev);
+
+if (!netdev_vport_is_vport_class(netdev->netdev_class)) {
+netdev_ports_traverse(netdev->dpif_type, flush_in_vport_cb, netdev);
+}
return 0;
}
--
2.25.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
