This code in odp-util is basically unmaintainable. I've never been able to figure out how to properly test or reason about it. It's a huge reason that I want to get rid of the kernel module.
On Mon, Oct 04, 2021 at 11:49:41AM +0200, Eelco Chaudron wrote: > Hi Ben, > > Any chance you can take a peek at the email below? I hope you can remember > this from 2018 :) > > Thanks, > > Eelco > > On 15 Sep 2021, at 15:30, Eelco Chaudron wrote: > > > Hi Ben, > > > > The mentioned fix below is causing a regression, actually two related ones: > > > > 1) If IGMP snooping is disabled, IGMP related traffic is still sent slow > > path for processing with the NORMAL rule. In addition, even without the > > NORMAL rule, the DP rule gets modified. > > > > Here are some examples: > > > > a) default config, one bridge (kernel dp), two ports (i.e. only a single > > NORMAL rule, and mcast_snooping_enable: false), send an IGMP join: > > > > - With your fix the dp rule looks like this: > > > > > > recirc_id(0),in_port(2),eth(src=04:f4:bc:28:57:01,dst=01:00:5e:00:00:16),eth_type(0x0800),ipv4(frag=no), > > packets:8, bytes:576, used:0.169s, > > actions:userspace(pid=4220974525,slow_path(match)) > > > > - Without the fix, which is correct: > > > > > > recirc_id(0),in_port(2),eth(src=04:f4:bc:28:57:01,dst=01:00:5e:00:00:16),eth_type(0x0800),ipv4(frag=no), > > packets:8, bytes:576, used:0.004s, actions:1 > > > > > > b) with a more specific OpenFlow entry, no DP entry gets created: > > > > ovs-ofctl del-flows ovs_pvp_br0 && ovs-ofctl add-flow ovs_pvp_br0 > > "in_port=enp5s0f0,eth_type(0x800),ip_proto=2,action=drop" > > ovs-ofctl del-flows ovs_pvp_br0 && ovs-ofctl add-flow ovs_pvp_br0 > > "in_port=enp5s0f0,eth_type(0x800),ip_proto=2,action=vnet6" > > > > > > 2) With the netdev (DPDK) interface all seems to work as expected, i.e., > > the flow gets installed correctly without the userspace upcall in both > > cases above. However, the verifier generates an error as it would like to > > update the dp rule with a slow_path variant. Which is failing as netdev is > > using the odp_flow_key_to_flow() utility function, which has the IGMP check. > > > > Looking at the change it assumes you filter on igmp type/code when you > > filter on the protocol type igmp. But this is not true, you can just filter > > on the protocol type only, and this is what is actually causing the > > problems above. So I think your patch needs to be reversed! > > > > However, looking at fixing Huanle's original problem, > > https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343665.html, > > looking at his patch, this might be the right solution. All IGMP packets > > independent of their type need to be processed by a SLOW_ACTION upcall. > > > > > > I've copied in Flavio, who worked on the IGMP snooping part before, as he > > might still remember what I argue above is true :) > > > > > > If you have any other ideas, thoughts please let me know. > > > > > > Cheers, > > > > Eelco > > > > ============================================================ > > > > commit c645550bb2498fb3816b6a39b22bffeb3154dca3 > > Author: Ben Pfaff <b...@ovn.org> > > Date: Wed Jan 24 11:40:20 2018 -0800 > > > > odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP. > > > > OVS datapaths don't understand or parse IGMP fields, but OVS userspace > > does, so this commit updates odp_flow_key_to_flow() to report that > > properly > > to the caller. > > > > Reported-by: Huanle Han <hanxue...@gmail.com> > > Reported-at: > > https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343665.html > > Signed-off-by: Ben Pfaff <b...@ovn.org> > > > > diff --git a/lib/odp-util.c b/lib/odp-util.c > > index 14d5af097..5da83b4c6 100644 > > --- a/lib/odp-util.c > > +++ b/lib/odp-util.c > > @@ -6210,6 +6210,11 @@ parse_l2_5_onward(const struct nlattr > > *attrs[OVS_KEY_ATTR_MAX + 1], > > } > > } > > } > > + } else if (src_flow->nw_proto == IPPROTO_IGMP > > + && src_flow->dl_type == htons(ETH_TYPE_IP)) { > > + /* OVS userspace parses the IGMP type, code, and group, but its > > + * datapaths do not, so there is always missing information. */ > > + return ODP_FIT_TOO_LITTLE; > > } > > if (is_mask && expected_bit != OVS_KEY_ATTR_UNSPEC) { > > if ((flow->tp_src || flow->tp_dst) && flow->nw_proto != 0xff) { > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
