Re: [ovs-dev] [PATCH] ofp-parse: Support "igmp" keyword in flows.
On Wed, Nov 04, 2020 at 05:33:27PM +0100, Ilya Maximets wrote: > On 11/3/20 5:46 PM, Ben Pfaff wrote: > > On Tue, Nov 03, 2020 at 03:53:51AM +0100, Ilya Maximets wrote: > >> On 11/3/20 12:28 AM, Ben Pfaff wrote: > >>> match_format() prints out "igmp" for IGMP flows, but > >>> ofp_parse_protocol() didn't accept it, which meant that OVS would print > >>> out a flow that it wouldn't re-parse. This fixes the problem and adds > >>> a test. > >> > >> I'm a bit confused. IIUC, matching on igmp is not supported by any > >> OF version or by any extensions, i.e. we could match by 'ip,nw_proto=2', > >> but there is no special OF header for "igmp" or "igmp_type" or "igmp_code". > >> While it seems easy to add parsing of "igmp" itself, its fields > >> ("igmp_type" > >> and "igmp_code") could appear in the output too and parsing of these fields > >> will, I guess, require a new OF extension. Is it correct? If so, maybe > >> it's better to remove "igmp*" printing code from the match_format() > >> instead? > > > > I hadn't thought about that. I was just thinking of this as a short > > form of "ip,nw_proto=2". > > > > It looks like OVS's support for IGMP matching is inconsistent. > > flow_extract() pulls the igmp_type and igmp_code values into tp_src and > > tp_dst. The kernel datapath doesn't do that, though, and there's no > > support for the fields at the OpenFlow level. I guess it has never been > > important enough. > > From my understanding, for now we only need support inside OVS userspace > for multicast snooping. So, OVS just instructs datapath that all igmp > (ip,nw_proto=2) packets should go to userspace, so they could be specially > processed. And this is just a configuration of the bridge, so no need > to have support in OpenFlow. I think, the same we have for LLDP: OVS > instructs datapath that all 0x88cc packets should go to userspace and > userspace handles these packets adjusting the configuration of ports and > bridges. In this case we don't really need any special support from the > datapath and this also works without involving the OF level as these > packets are consumed by OVS itself and not passed through OF pipelines. > > > > > OK, I won't complain either way then. > > I think, it's better to just stop printing word 'igmp' in flow dumps to > avoid confusion since OF doesn't support igmp. We're not printing anything > like this for lldp or some other types of packets anyway. > > Flavio, you were the original author of this mcast snooping feature, what > do you think? Sorry the delay. Your understanding is correct about the membership packets going to userspace for special processing. There was no intention of exposing the protocol itself. I don't like changing outputs because doing so can break scripts but maybe in this case that's better in order to keep consistency with LLDP and OF. -- fbl ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofp-parse: Support "igmp" keyword in flows.
On 11/3/20 5:46 PM, Ben Pfaff wrote: > On Tue, Nov 03, 2020 at 03:53:51AM +0100, Ilya Maximets wrote: >> On 11/3/20 12:28 AM, Ben Pfaff wrote: >>> match_format() prints out "igmp" for IGMP flows, but >>> ofp_parse_protocol() didn't accept it, which meant that OVS would print >>> out a flow that it wouldn't re-parse. This fixes the problem and adds >>> a test. >> >> I'm a bit confused. IIUC, matching on igmp is not supported by any >> OF version or by any extensions, i.e. we could match by 'ip,nw_proto=2', >> but there is no special OF header for "igmp" or "igmp_type" or "igmp_code". >> While it seems easy to add parsing of "igmp" itself, its fields ("igmp_type" >> and "igmp_code") could appear in the output too and parsing of these fields >> will, I guess, require a new OF extension. Is it correct? If so, maybe >> it's better to remove "igmp*" printing code from the match_format() instead? > > I hadn't thought about that. I was just thinking of this as a short > form of "ip,nw_proto=2". > > It looks like OVS's support for IGMP matching is inconsistent. > flow_extract() pulls the igmp_type and igmp_code values into tp_src and > tp_dst. The kernel datapath doesn't do that, though, and there's no > support for the fields at the OpenFlow level. I guess it has never been > important enough. >From my understanding, for now we only need support inside OVS userspace for multicast snooping. So, OVS just instructs datapath that all igmp (ip,nw_proto=2) packets should go to userspace, so they could be specially processed. And this is just a configuration of the bridge, so no need to have support in OpenFlow. I think, the same we have for LLDP: OVS instructs datapath that all 0x88cc packets should go to userspace and userspace handles these packets adjusting the configuration of ports and bridges. In this case we don't really need any special support from the datapath and this also works without involving the OF level as these packets are consumed by OVS itself and not passed through OF pipelines. > > OK, I won't complain either way then. I think, it's better to just stop printing word 'igmp' in flow dumps to avoid confusion since OF doesn't support igmp. We're not printing anything like this for lldp or some other types of packets anyway. Flavio, you were the original author of this mcast snooping feature, what do you think? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofp-parse: Support "igmp" keyword in flows.
On Tue, Nov 03, 2020 at 03:53:51AM +0100, Ilya Maximets wrote: > On 11/3/20 12:28 AM, Ben Pfaff wrote: > > match_format() prints out "igmp" for IGMP flows, but > > ofp_parse_protocol() didn't accept it, which meant that OVS would print > > out a flow that it wouldn't re-parse. This fixes the problem and adds > > a test. > > I'm a bit confused. IIUC, matching on igmp is not supported by any > OF version or by any extensions, i.e. we could match by 'ip,nw_proto=2', > but there is no special OF header for "igmp" or "igmp_type" or "igmp_code". > While it seems easy to add parsing of "igmp" itself, its fields ("igmp_type" > and "igmp_code") could appear in the output too and parsing of these fields > will, I guess, require a new OF extension. Is it correct? If so, maybe > it's better to remove "igmp*" printing code from the match_format() instead? I hadn't thought about that. I was just thinking of this as a short form of "ip,nw_proto=2". It looks like OVS's support for IGMP matching is inconsistent. flow_extract() pulls the igmp_type and igmp_code values into tp_src and tp_dst. The kernel datapath doesn't do that, though, and there's no support for the fields at the OpenFlow level. I guess it has never been important enough. OK, I won't complain either way then. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofp-parse: Support "igmp" keyword in flows.
On 11/3/20 12:28 AM, Ben Pfaff wrote: > match_format() prints out "igmp" for IGMP flows, but > ofp_parse_protocol() didn't accept it, which meant that OVS would print > out a flow that it wouldn't re-parse. This fixes the problem and adds > a test. I'm a bit confused. IIUC, matching on igmp is not supported by any OF version or by any extensions, i.e. we could match by 'ip,nw_proto=2', but there is no special OF header for "igmp" or "igmp_type" or "igmp_code". While it seems easy to add parsing of "igmp" itself, its fields ("igmp_type" and "igmp_code") could appear in the output too and parsing of these fields will, I guess, require a new OF extension. Is it correct? If so, maybe it's better to remove "igmp*" printing code from the match_format() instead? Best regards, Ilya Maximets. > > Signed-off-by: Ben Pfaff > --- > lib/ofp-parse.c| 1 + > tests/ovs-ofctl.at | 2 ++ > 2 files changed, 3 insertions(+) > > diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c > index a90b926efb54..395a519f555f 100644 > --- a/lib/ofp-parse.c > +++ b/lib/ofp-parse.c > @@ -184,6 +184,7 @@ ofp_parse_protocol(const char *name, const struct > ofp_protocol **p_out) > { "ip4", ETH_TYPE_IP, 0 }, > { "arp", ETH_TYPE_ARP, 0 }, > { "icmp", ETH_TYPE_IP, IPPROTO_ICMP }, > +{ "igmp", ETH_TYPE_IP, IPPROTO_IGMP }, > { "tcp", ETH_TYPE_IP, IPPROTO_TCP }, > { "udp", ETH_TYPE_IP, IPPROTO_UDP }, > { "sctp", ETH_TYPE_IP, IPPROTO_SCTP }, > diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at > index b6951f404c45..05ba943d84d6 100644 > --- a/tests/ovs-ofctl.at > +++ b/tests/ovs-ofctl.at > @@ -191,6 +191,7 @@ actions=note:41.42.43,note:00.01.02.03.04.05.06.07,note > ip,actions=set_field:10.4.3.77->ip_src,mod_nw_ecn:2 > sctp actions=drop > sctp actions=drop > +igmp actions=drop > in_port=0 actions=resubmit:0 > > actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678) > > actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678,ingress) > @@ -225,6 +226,7 @@ OFPT_FLOW_MOD: ADD > actions=note:41.42.43.00.00.00,note:00.01.02.03.04.05.06.07.0 > OFPT_FLOW_MOD: ADD ip actions=mod_nw_src:10.4.3.77,load:0x2->NXM_NX_IP_ECN[] > OFPT_FLOW_MOD: ADD sctp actions=drop > OFPT_FLOW_MOD: ADD sctp actions=drop > +OFPT_FLOW_MOD: ADD igmp actions=drop > OFPT_FLOW_MOD: ADD in_port=0 actions=resubmit:0 > OFPT_FLOW_MOD: ADD > actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678) > OFPT_FLOW_MOD: ADD > actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678,ingress) > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ofp-parse: Support "igmp" keyword in flows.
match_format() prints out "igmp" for IGMP flows, but ofp_parse_protocol() didn't accept it, which meant that OVS would print out a flow that it wouldn't re-parse. This fixes the problem and adds a test. Signed-off-by: Ben Pfaff --- lib/ofp-parse.c| 1 + tests/ovs-ofctl.at | 2 ++ 2 files changed, 3 insertions(+) diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index a90b926efb54..395a519f555f 100644 --- a/lib/ofp-parse.c +++ b/lib/ofp-parse.c @@ -184,6 +184,7 @@ ofp_parse_protocol(const char *name, const struct ofp_protocol **p_out) { "ip4", ETH_TYPE_IP, 0 }, { "arp", ETH_TYPE_ARP, 0 }, { "icmp", ETH_TYPE_IP, IPPROTO_ICMP }, +{ "igmp", ETH_TYPE_IP, IPPROTO_IGMP }, { "tcp", ETH_TYPE_IP, IPPROTO_TCP }, { "udp", ETH_TYPE_IP, IPPROTO_UDP }, { "sctp", ETH_TYPE_IP, IPPROTO_SCTP }, diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at index b6951f404c45..05ba943d84d6 100644 --- a/tests/ovs-ofctl.at +++ b/tests/ovs-ofctl.at @@ -191,6 +191,7 @@ actions=note:41.42.43,note:00.01.02.03.04.05.06.07,note ip,actions=set_field:10.4.3.77->ip_src,mod_nw_ecn:2 sctp actions=drop sctp actions=drop +igmp actions=drop in_port=0 actions=resubmit:0 actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678) actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678,ingress) @@ -225,6 +226,7 @@ OFPT_FLOW_MOD: ADD actions=note:41.42.43.00.00.00,note:00.01.02.03.04.05.06.07.0 OFPT_FLOW_MOD: ADD ip actions=mod_nw_src:10.4.3.77,load:0x2->NXM_NX_IP_ECN[] OFPT_FLOW_MOD: ADD sctp actions=drop OFPT_FLOW_MOD: ADD sctp actions=drop +OFPT_FLOW_MOD: ADD igmp actions=drop OFPT_FLOW_MOD: ADD in_port=0 actions=resubmit:0 OFPT_FLOW_MOD: ADD actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678) OFPT_FLOW_MOD: ADD actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678,ingress) -- 2.26.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev