On Thu, Jun 9, 2016 at 5:25 AM, Ben Pfaff <b...@ovn.org> wrote:
> On Wed, Jun 08, 2016 at 03:26:30PM +0800, Zong Kai LI wrote:
> > This patch adds a new OVN action 'na' to support ND versus ARP.
> >
> > When ovn-controller received a ND packet, it frames a NA packet for
> > reply, with mac address parsed from userdata as eth.dst. Then it
> > reloads metadata info from previous packet to framed packet, and
> > finally send the framed packet back with left actions.
> >
> > Eg. na{12:34:56:78:9a:bc; reg0 = 0x1; outport = inport; inport = "";
> output;};
> >
> > Since patch port for IPv6 router interface is not ready yet, this
> > patch will only try to deal with ND from VM. This patch will set
> > RSO flags to 011 for NA packets.
> >
> > The next patch will do logical flows works for this action.
> >
> > Signed-off-by: Zong Kai LI <zealo...@gmail.com>
>
> Thanks for the patch!
>
> I think it would be better to retain more of the flavor of the arp
> action here. For that action, it is the responsibility of whoever
> writes the flow to initialize anything that cannot be inferred by the
> action. For example, "arp" does not take a parameter that is assigned
> to eth.src; instead, whoever writes the flow simply puts an assignment
> to eth.src inside the nested set of actions. That's more flexible, too,
> because it allows eth.src to come from someplace other than an immediate
> constant.
>
> The intent with OVN logical actions, by the way, is that only actions
> should be enclosed in {}; data parameters should be enclosed in (). It
> looks funny to me to mix both of them inside {}. But if we drop the
> eth.src parameter then that solves the problem.
>
> The documentation for reg0, outport, and inport seems strange, because
> it implies that the action actually changes these. It doesn't, as far
> as I can tell.
>
> I still don't see any attempt to deal with alignment issues for RISC
> machines. Using ALIGNED_CAST does not solve a problem, it only
> suppresses a warning.
>
> I'm appending the changes that I recommend for the documentation. My
> changes do not actually implement these changes, only document them. My
> changes also include some stylistic fixes to better match
> CodingStyle.md.
>
> Thanks,
>
> Ben.
>
> --8<--------------------------cut here-------------------------->8--
>
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 1af9e89..79c4070 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -765,7 +765,7 @@ pinctrl_handle_na(const struct flow *ip_flow,
> goto exit;
> }
>
> - // Frame the NA packet with RSO=011.
> + /* Frame the NA packet with RSO=011. */
> uint64_t packet_stub[128 / 8];
> struct dp_packet packet;
> dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
> @@ -774,7 +774,7 @@ pinctrl_handle_na(const struct flow *ip_flow,
> &(ip_flow->nd_target), &(ip_flow->ipv6_src),
> htons(0x6000));
>
> - // Reload previous packet metadata.
> + /* Reload previous packet metadata. */
> uint64_t ofpacts_stub[4096 / 8];
> struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
> reload_metadata(&ofpacts, md);
> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index 5189401..3fd626f 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -986,7 +986,7 @@
> </dd>
>
> <dt>
> - <code>na{<var>A</var>; <var>action</var>; </code>...<code>
> };</code>
> + <code>na { <var>action</var>; </code>...<code> };</code>
> </dt>
>
> <dd>
> @@ -999,38 +999,29 @@
>
> <p>
> The NA packet that this action operates on is initialized
> based on
> - the IPv6 packet being processed(with userdata), as follows:
> + the IPv6 packet being processed, as follows. These are
> default
> + values that the nested actions will probably want to change:
> </p>
>
> <ul>
> - <li><code>eth.dst</code> copied from eth.src</li>
> - <li><code>eth.src</code> copied from userdata</li>
> + <li><code>eth.dst</code> copied from
> <code>eth.src</code>.</li>
> + <li><code>eth.src</code> unchanged</li>
> <li><code>eth.type = 0x86dd</code></li>
> <li><code>ip6.dst</code> copied from <code>ip6.src</code></li>
> <li><code>ip6.src</code> copied from
> <code>nd.target</code></li>
> <li><code>icmp6.type = 136</code> (Neighbor
> Advertisement)</li>
> <li><code>nd.target</code> unchanged</li>
> <li><code>nd.sll = 00:00:00:00:00:00</code></li>
> - <li><code>nd.sll</code> copied from userdata</li>
> + <li><code>nd.sll</code> unchanged</li>
> </ul>
>
> <p>
> - These are default values that the nested actions will
> probably want
> - to change:
> - <p>
> -
> - <ul>
> - <li><code>reg0 = 0x1</code>(Mark as replied by
> ovn-controller)</li>
> - <li><code>outport</code> copied from inport</li>
> - <li><code>inport = ""</code></li>
> - </ul>
> -
> The ND packet has the same VLAN header, if any, as the IP
> packet
> it replaces.
> </p>
>
> <p>
> - <b>Prerequisite:</b> <code>nd&&icmp6.type ==
> 135</code>
> + <b>Prerequisite:</b> <code>nd && icmp6.type ==
> 135</code>
> </p>
> </dd>
>
>
Hi, Ben.
Thanks for your time and review.
Sure, I will try to work on separate parameters and actions, your
suggestions make sense to me.
And I will also update documentation for reg0, inport, outport.
About alignment issues, I will think about that.
Thanks again and have a nice day! :)
Best regards,
Zong Kai, LI
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
