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

[Numan Siddique]

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".

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 ?

Thanks
Numan



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