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

[Jesse Gross]

On Thu, Jun 13, 2013 at 7:32 PM, Andy Zhou <azhou at nicira.com> wrote:
> 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.

Hmm, I suppose this is true. Never mind then.

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

What's the difference here?

>> >  {
>> > +       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.

Doesn't returning false here signify that this is not a 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.

Branching is usually very inefficient in cases where it is hard to predict.

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

ovs_sw_flow_mask_del_ref() is called from the flow RCU callback. It
then manipulates the list without holding a lock.