[ovs-dev] [PATCHv3 2/6] odp-util: Specify export features when serialising flows.

[Joe Stringer]

On 16 May 2014 15:38, Andy Zhou <azhou at nicira.com> wrote:

> On Thu, May 15, 2014 at 8:34 PM, Joe Stringer <joestringer at nicira.com>
> wrote:
> > On 16 May 2014 15:11, Andy Zhou <azhou at nicira.com> wrote:
> >>
> >> On Wed, May 14, 2014 at 11:57 PM, Joe Stringer <joestringer at nicira.com>
> >> wrote:
> >> > Signed-off-by: Joe Stringer <joestringer at nicira.com>
> >> > ---
> >> > v3: First post.
> >> > ---
> >> >  lib/odp-util.c |   41 +++++++++++++++++++++++++----------------
> >> >  1 file changed, 25 insertions(+), 16 deletions(-)
> >> >
> >> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> >> > index 6cff2f1..ff3cb7e 100644
> >> > --- a/lib/odp-util.c
> >> > +++ b/lib/odp-util.c
> >> > @@ -2482,16 +2482,16 @@ ovs_to_odp_frag_mask(uint8_t nw_frag_mask)
> >> >
> >> >  static void
> >> >  odp_flow_key_from_flow__(struct ofpbuf *buf, const struct flow *flow,
> >> > -                         const struct flow *mask, odp_port_t
> >> > odp_in_port,
> >> > -                         size_t max_mpls_depth, bool export_mask)
> >> > +                         const struct flow *mask, const struct flow
> >> > *data,
> >> > +                         odp_port_t odp_in_port, size_t
> max_mpls_depth,
> >> > +                         uint64_t export_mask)
> >> Changing export_mask from bool to uint64_t seems to be overkill.  I
> >> hope we can avoid doing this.
> >
> >
> > I realise I didn't explain how this should work, but it was an attempt at
> > simplifying this code (if perhaps a little misguided). Rather than
> passing
> > several parameters to determine special cases for the function, it would
> be
> > nice to be able to specify separately what to serialise and what to not.
> >
> >
> >> How about we the following prototype:
> >> odp_flow_key_from_flow__(struct ofpbuf *buf, const struct flow *flow,
> >>                          const struct flow *mask, odp_port_t
> odp_in_port,
> >>                          size_t max_mpls_depth, bool dp_recirc, bool
> >> export_mask)
> >> --------------------------------------------------------->
> >> ^^^^^^^^^^^^^^^
> >>
> >> /* dp_recirc indicates that datapath understands recirculation */
> >>
> >> The decision of exporting recirc_id and dp_hash will be simple:
> >>
> >>      if (dp_recric) {
> >>            nl_msg_put_u32(buf, OVS_KEY_ATTR_RECIRC_ID ...)
> >>            nl_msg_put_u32(buf, OVS_KEY_ATTR_DP_HASH ... )
> >>      }
> >>
> >> We can obtain dp_recirc value similar to max_mpls_depth.
> >>
> >> For example: in ofproto-dpif-upcall.c  we can call
> >> ofproto_dpif_get_enable_recirc() to obtain the value.
> >>
> >> dp_recirc can always be set to true within the user space datapath.
> >>
> >> Would this work?
> >
> >
> > I think this would work. We already store the value for dp_recirc, which
> can
> > be retrieved by ofproto_dpif_get_enable_recirc(). If you're happy with
> that,
> > I can resend.
> Thanks. assuming this works for both kernel and user space datapath.
>

I think your proposal is equivalent in behaviour to this patch, which I
tested against an older datapath in the basic case. I'll plan to test that
for the new version.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140516/2fd9fc87/attachment-0005.html>