[ovs-dev] [PATCH ovn 1/4] ovn-controller: A new action "select".

Han Zhou hzhou at ovn.org
Mon Jan 20 23:06:45 UTC 2020


On Mon, Jan 20, 2020 at 6:34 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Sat, Jan 18, 2020 at 3:28 AM Han Zhou <hzhou at ovn.org> wrote:
> >
> > On Fri, Jan 17, 2020 at 4:38 AM Numan Siddique <numans at ovn.org> wrote:
> > >
> > > On Tue, Jan 7, 2020 at 8:00 AM Han Zhou <hzhou at ovn.org> wrote:
> > > >
> > > > Support a new logical flow action "select", which can be used to
> > > > implement features such as ECMP.  The action uses OpenFlow group
> > > > action to select an integer (uint16_t) from a list of integers,
> > > > and assign it to specified field, e.g.:
> > > >     select(reg0, 1, 2, 3)
> > > > A weight can be specified for each member as well, e.g.:
> > > >     select(reg0, 1=20, 2=30, 3=50)
> > > >
> > > > Signed-off-by: Han Zhou <hzhou at ovn.org>
> > >
> > >
> > > Hi Han,
> > >
> > > The overall series LGTM. I have one comment on the action "select".
> > > We already have existing actions  like put_dhcp_opts (and others)
> > > which store the result to a register.
> > > I think it would be better to use it in similar way i.e "reg0 =
> > > select(1:20, 2:30, 3:50)".
> > >
> > Thanks Numan!
> >
> > In fact I was trying to do exactly the same as what you are suggesting.
> > However, the OF group action is special that the actions executed in
group
> > buckets can't save any changes made to registers, metadata or stack out
of
> > the action execution. See documentation details in this commit:
> >
https://github.com/openvswitch/ovs/commit/afc0c379f6e13cdcbb4447f17bbb74b882503af4
> >
> > Because of this, we can't exit the action, and resubmit is used as the
last
> > action in each bucket to recursively execute the next pipelines with the
> > new values of the registers. For the same reason, we have to change the
> > register inside the bucket, instead of using "=" outside of the action.
> >
> > > Also instead of "=" I would suggest to use ":" to store the weight.
> >
> > I was also using ":" in the beginning, but it causes problem in lexer
> > parsing, because the lex_parse_integer() treats ":" as part of a token
so
> > that it can parse a MAC address or a <IP>:<port> token (although these
are
> > not integers).
> > /* Find the extent of an "integer" token, which can be in decimal or
> >  * hexadecimal, or an Ethernet address or IPv4 or IPv6 address, as
'start'
> >  * through 'end'.
> > ...
> >
> > So this is why I changed to use "=" instead.
> > Does this make sense to you?
>
> Hi Han,
>
> I am a little confused. I am not suggesting to change anything on the
> openflow part.
>
> In my testing, I added 2 routes -
>
> ovn-nbctl lr-route-add lr0 172.23.0.0/24 10.0.0.3
> ovn-nbctl --ecmp lr-route-add lr0 172.23.0.0/24 10.0.0.4
>
> And I see the below logical flows
>
> ***
>  table=9 (lr_in_ip_routing   ), priority=49   , match=(ip4.dst ==
> 172.23.0.0/24), action=(ip.ttl--; flags.loopback = 1; reg8[0..15] = 1;
> select(reg8[16..31], 1, 2);)
>  ...
>   table=10(lr_in_ip_routing_ecmp), priority=150  , match=(reg8[0..15]
> == 0), action=(next;)
>   table=10(lr_in_ip_routing_ecmp), priority=100  , match=(reg8[0..15]
> == 1 && reg8[16..31] == 1), action=(reg0 = 10.0.0.4; reg1 = 10.0.0.1;
> eth.src = 00:00:00:00:ff:01; outport = "lr0-sw0"; next;)
>   table=10(lr_in_ip_routing_ecmp), priority=100  , match=(reg8[0..15]
> == 1 && reg8[16..31] == 2), action=(reg0 = 10.0.0.3; reg1 = 10.0.0.1;
> eth.src = 00:00:00:00:ff:01; outport = "lr0-sw0"; next;)
> ***
>
> My suggestion is to have logical flow something like below
> ****
> table=9 (lr_in_ip_routing   ), priority=49   , match=(ip4.dst ==
> 172.23.0.0/24), action=(ip.ttl--; flags.loopback = 1; reg8[0..15] = 1;
> reg8[16..31] = select(1, 2);)
> ...
> ..
> ...
> ****
>
> You just need to only handle this in parse_select_action(). Rest of
> the code would remain the same including
> encode_SELECT(). In parse_select_action(), you need to store the
> "res_field" of the "struct ovnact_select" appropriately
> as the result register will be now before "= select".
>
Ok, sorry for my misunderstanding. I thought you were suggesting to change
the openflow encoding to "=" outside of the group buckets as well.
I changed as what you suggested in v2, please take a look:
https://patchwork.ozlabs.org/patch/1226173/
Patch 4 in the series also changed so that northd uses the new format of
select action.

> Functionally nothing changes, just that it will be clearer to see the
> logical flow.
>
> Also any particular reason to choose 16-bit reg8[0..15] to indicate
> that its ecmp ? in table 9. It can just be 1-bit right ?
>
The reg8[0..15], i.e. REG_ECMP_GROUP_ID, is used to store the ECMP group's
ID instead of as a flag. There can be many ECMP route groups. In the next
table, it matches both "group ID" and "member ID in group" to decide what
routing actions should be taken. So it can't be 1-bit. Assigning it to 0
for non-ECMP routes is just for efficiency so that in table 10 it can
directly match group ID 0 and go to next, because real group IDs start from
1.

> Thanks
> Numan
>
>
>
> >
> > Thanks,
> > Han
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >


More information about the dev mailing list