Thanks for the review, will send a v2 Eelco Chaudron <[email protected]>于2023年6月8日 周四21:48写道：
> > > On 2 Jun 2023, at 20:42, Peng He wrote: > > > 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 <[email protected]> > > Hi Peng, > > This patch looks good in general, some style comments, and a problem with > the tests. > > Thanks, > > Eelco > > > --- > > lib/dpif-netdev.c | 64 ++++++++++++++++++++++++++--------------------- > > tests/pmd.at | 55 ++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 90 insertions(+), 29 deletions(-) > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index 70b953ae6..1362f27e6 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); > > + 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); > > Indentation is off here, and below. > > > > > - 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 before outputting the > stats */ > > - error = EOPNOTSUPP; > > + error = EOPNOTSUPP; > > + } > > + ovsrcu_postpone(dp_netdev_actions_free, old_actions); > > } > > - > > - ovsrcu_postpone(dp_netdev_actions_free, old_actions); > > - } else if (put->flags & DPIF_FP_CREATE) { > > - error = EEXIST; > > } else { > > - /* Overlapping flow. */ > > - error = EINVAL; > > + if (netdev_flow) { > > + /* Overlapping flow. */ > > + error = EINVAL; > > In general, as we only take care of DPIF_FP_CREATE or DPIF_FP_MODIFY we > could simplify the if/then tree. > Something like: > > if (!(put->flags & DPIF_FP_CREATE || put->flags & DPIF_FP_MODIFY)) { > return EINVAL > } > > > if (put->flags & DPIF_FP_CREATE) { > > > } else { > > > } > > > + } > > } > > } > > - ovs_mutex_unlock(&pmd->flow_mutex); > > return error; > > } > > > > diff --git a/tests/pmd.at b/tests/pmd.at > > index 48f3d432d..8e18ed5dd 100644 > > --- a/tests/pmd.at > > +++ b/tests/pmd.at > > @@ -1300,3 +1300,58 @@ OVS_WAIT_UNTIL([tail -n +$LINENUM > ovs-vswitchd.log | grep "PMD load based sleeps > > > > OVS_VSWITCHD_STOP > > AT_CLEANUP > > + > > +AT_SETUP([pmd - revalidator wrongly modify userspace megaflows]) > OVS_VSWITCHD_START( > > PMD should be in capitals like with all other tests. > > Also OVS_VSWITCHD_START() should start on a new line. > > > + [add-port br0 p1 \ > > + -- set interface p1 type=dummy-pmd \ > > + -- set bridge br0 datapath-type=dummy \ > > + -- add-port br0 p2 \ > > + -- set interface p2 type=dummy-pmd -- > > + ]) > > + > > +AT_DATA([flows.txt], [dnl > > +table=0,in_port=p1,ip,nw_dst=10.1.2.0/24,actions=p2 > > +]) > > + > > +dnl replace openflow, let revalidator work > > Please rework all comments, start the sentence with a Capital and end with > a dot. > > > +AT_DATA([flows2.txt], [dnl > > +table=0,in_port=p1,ip,nw_dst=10.1.0.0/16 actions=ct(commit) > > +]) > > + > > +AT_DATA([flows3.txt], [dnl > > +table=0,in_port=p1,ip,nw_dst=10.1.0.0/16 actions=p2 > > +]) > > + > > +ovs-ofctl add-flows br0 flows.txt > > +ovs-appctl netdev-dummy/receive p1 > 'ipv4(src=10.0.0.1,dst=10.1.2.2),tcp(src=1,dst=2)' > > + > > +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*core: 0//'], [0], [ > > This regex needs changing as it’s failing on my system: > > +++ > /home/echaudron/Documents/review/ovs_quick/tests/testsuite.dir/at-groups/1084/stdout > 2023-06-08 14:52:39.256073655 +0200 > @@ -1,3 +1,3 @@ > - > +flow-dump from pmd on cpu core: 11 > recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst= > 10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:never, actions:2 > > > Maybe something like: > > -AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*core: 0//'], [0], [ > +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*core: [[0-9]]\+//'], > [0], [ > > Here and 2x more below. > > > > +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst= > 10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:never, actions:2 > > +]) > > + > > +ovs-appctl netdev-dummy/receive p1 > 'ipv4(src=10.0.0.1,dst=10.1.2.2),tcp(src=1,dst=2)' > > + > > +dnl trigger revalidating and wait > > +AT_CHECK([ovs-ofctl --bundle replace-flows br0 flows2.txt]) > > +ovs-appctl revalidator/wait > > + > > +ovs-appctl netdev-dummy/receive p1 > 'ipv4(src=10.0.0.1,dst=10.1.0.2),tcp(src=1,dst=2)' > > +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*core: 0//' | > strip_xout_keep_actions], [0], [ > > > +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst= > 10.1.0.2/255.255.0.0,frag=no), packets:0, bytes:0, used:never, > actions:ct(commit) > > > +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst= > 10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:0.0s, > actions:ct(commit) > > +]) > > + > > +dnl send more 10.1.0.2 to make 10.1.0.0/16 tuple preprend 10.1.2.0/24 > tuple in pvector of subtable > > +for i in `seq 0 512`;do > > for i in `seq 0 512`; do > > > +ovs-appctl netdev-dummy/receive p1 > 'ipv4(src=10.0.0.1,dst=10.1.0.2),tcp(src=1,dst=2)' > > Indent this comment. > > > +done > > + > > +AT_CHECK([ovs-ofctl --bundle replace-flows br0 flows3.txt]) > > + > > +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*core: 0//' | > strip_xout_keep_actions], [0], [ > > > +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst= > 10.1.0.2/255.255.0.0,frag=no), packets:0, bytes:0, used:0.0s, actions:2 > > > +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst= > 10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:0.0s, actions:2 > > +]) > > + > > +OVS_VSWITCHD_STOP > > +AT_CLEANUP > > -- > > 2.25.1 > > -- hepeng _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
