[ovs-dev] [PATCHv4] odp-util: Always serialise recirculation in upcall key.

[Joe Stringer]

Oh, I missed the Acked-by. Thanks, I'll fix that piece up and push to
master soon.

Would you mind in future, putting a bit more whitespace around your
comments and only keep the minimal context for replies? I find it a bit
difficult to see comments when there is so much unrelated text from the
patch.


On 19 May 2014 13:51, Joe Stringer <joestringer at nicira.com> wrote:

> On 19 May 2014 13:05, Andy Zhou <azhou at nicira.com> wrote:
>
>> On Thu, May 15, 2014 at 9:52 PM, Joe Stringer <joestringer at nicira.com>
>> wrote:
>> > The userspace and kernel datapaths previously differed on their
>> > treatment of the recirc_id and dp_hash fields when sending upcalls.
>> > While the kernel datapath would always serialise these fields, the
>> > userspace would not. When using the userspace datapath, this would cause
>> > a mismatch between the odp flow key in an upcall compared to the one
>> > that is serialised upon flow_dump.
>> >
>> > This patch brings the userspace datapath behaviour back in line with the
>> > kernel datapath by always serialising recirc_id and dp_hash to odp.
>> >
>> > Signed-off-by: Joe Stringer <joestringer at nicira.com>
>> Acked-by: Andy Zhou <azhou at nicira.com>
>> > ---
>> > v4: Simplify. Pass only a new "recirc" support parameter to
>> >       odp_flow_key_from_flow__().
>> > v3: Allow users to specify context for serialising fields.
>> >     Don't always match dp_hash.
>> > v2: Always match dp_hash.
>> > v1: Initial post.
>> > ---
>> >  lib/dpif-netdev.c             |    6 +-
>> >  lib/odp-util.c                |   30 ++++++----
>> >  lib/odp-util.h                |    5 +-
>> >  ofproto/ofproto-dpif-upcall.c |    5 +-
>> >  ofproto/ofproto-dpif.c        |    4 +-
>> >  tests/odp.at                  |   20 +++----
>> >  tests/ofproto-dpif.at         |  126
>> ++++++++++++++++++++---------------------
>> >  tests/test-odp.c              |    2 +-
>> >  8 files changed, 104 insertions(+), 94 deletions(-)
>> >
>> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> > index a255a96..d3f9c7e 100644
>> > --- a/lib/dpif-netdev.c
>> > +++ b/lib/dpif-netdev.c
>> > @@ -1455,7 +1455,7 @@ dpif_netdev_flow_dump_next(const struct dpif
>> *dpif, void *iter_, void *state_,
>> >
>> >          ofpbuf_use_stack(&buf, &state->keybuf, sizeof state->keybuf);
>> >          odp_flow_key_from_flow(&buf, &netdev_flow->flow, &wc.masks,
>> > -                               netdev_flow->flow.in_port.odp_port);
>> > +                               netdev_flow->flow.in_port.odp_port,
>> true);
>> >
>> >          *key = ofpbuf_data(&buf);
>> >          *key_len = ofpbuf_size(&buf);
>> > @@ -1467,7 +1467,7 @@ dpif_netdev_flow_dump_next(const struct dpif
>> *dpif, void *iter_, void *state_,
>> >          ofpbuf_use_stack(&buf, &state->maskbuf, sizeof state->maskbuf);
>> >          odp_flow_key_from_mask(&buf, &wc.masks, &netdev_flow->flow,
>> >                                 odp_to_u32(wc.masks.in_port.odp_port),
>> > -                               SIZE_MAX);
>> > +                               SIZE_MAX, true);
>> >
>> >          *mask = ofpbuf_data(&buf);
>> >          *mask_len = ofpbuf_size(&buf);
>> > @@ -2063,7 +2063,7 @@ dp_netdev_output_userspace(struct dp_netdev *dp,
>> struct ofpbuf *packet,
>> >
>> >          /* Put ODP flow. */
>> >          miniflow_expand(key, &flow);
>> > -        odp_flow_key_from_flow(buf, &flow, NULL,
>> flow.in_port.odp_port);
>> > +        odp_flow_key_from_flow(buf, &flow, NULL,
>> flow.in_port.odp_port, true);
>> >          upcall->key = ofpbuf_data(buf);
>> >          upcall->key_len = ofpbuf_size(buf);
>> >
>> > diff --git a/lib/odp-util.c b/lib/odp-util.c
>> > index 6cff2f1..37ec2be 100644
>> > --- a/lib/odp-util.c
>> > +++ b/lib/odp-util.c
>> > @@ -2483,7 +2483,8 @@ 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)
>> > +                         size_t max_mpls_depth, bool export_mask,
>> > +                         bool recirc)
>> How about keeping bool export_mask as the last parameter?
>> max_mpls_depth and recirc logically belong together.
>
>
> Sure. Would you like me to send a fresh version with this change for
> review?
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140519/eff4a88e/attachment-0005.html>