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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
