[ovs-dev] [megaflow v4 1/2] datapath: Mega flow implementation

Andy Zhou azhou at nicira.com
Fri Jun 14 02:32:49 UTC 2013


Thanks for the review.  A few comments inline:

> +static void flow_key_mask(struct sw_flow_key *dst,
> > +                         const struct sw_flow_key *src,
> > +                         const struct sw_flow_mask *mfm)
> > +{
> > +       u8 *m = (u8 *)&mfm->key + mfm->range.start;
> > +       u8 *s = (u8 *)src + mfm->range.start;
> > +       u8 *d = (u8 *)dst + mfm->range.start;
> > +       int i;
> > +
> > +       memset(dst, 0, sizeof(*dst));
> > +       for (i = 0; i < ovs_sw_flow_mask_size_roundup(mfm); i++) {
> > +               *d = *s & *m;
> > +               d++, s++, m++;
> > +       }
> > +}
>
> What if we just did an in-place masking? This would avoid the need to
> assume the hash algorithm's alignment requirements or do any roundup
> at all.
>

What do you mean by 'in-pace'?  It seems we will need to preserve the 'src'
key value so that we can apply the next mask.

>
> >  static int parse_icmpv6(struct sk_buff *skb, struct sw_flow_key *key,
> > -                       int *key_lenp, int nh_len)
> > +                       int nh_len)
> [...]
> >  out:
> > -       *key_lenp = key_len;
> >         return error;
>
> We can probably just remove the 'out' label altogether now.
>
It does not seem to improve readability of removing this out.   Removing
'out' from ovs_flow_extract makes sense.

>
> The parentheses around the return clause aren't really necessary.
>
> > +static bool is_zero_field(const u8* fp, size_t size)
>
> Please put a space before the asterisk.
>
> >  {
> > +       if (!fp)
> > +               return false;
>
> Does it make more logical sense for a NULL field to be considered
> zero? I'm not sure that we can ever really hit this case anyways.
>

When mask is not provided, we treat it as a wildcard fields, same as a all
zero mask.

>
> > +       for (i = 0; i < size; i++)
> > +               if (*(fp + i))
> > +                       return false;
>
> The nicest way of doing this to just OR everything together and check
> at the end.
>
This can be done for sure, but it may be less efficient. The same function
in the user space OVS code also returns as soon as a non-zero value is
detected.

>
> > +               mfm->ref_count--;
> > +
> > +               if (!mfm->ref_count) {
> > +                       list_del_rcu(&mfm->list);
> > +                       call_rcu(&mfm->rcu, rcu_free_sw_flow_mask_cb);
> > +               }
> > +       }
> > +}
>
> I'm not sure that it's actually necessary to use an RCU callback to
> track the lifetime of masks - you could unlink them from the list when
> the flow is deleted rather than from its RCU callback. Beyond
> potentially simplifying things, it fixes a bug - we manipulate the
> list and refcount from the flow RCU callback assuming that OVS lock is
> held but it isn't at that point (incidentally, lockdep should have
> flagged this so it might be worth running).
>
Not sure if I understand this completely.  list_del_rcu is called on flow
deletion,  The RCU callback is only for freeing memory, which should be
safe without holding the OVS lock. Let's discuss further when we meet.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130613/84d2636b/attachment-0003.html>


More information about the dev mailing list