On Thu, Jun 13, 2013 at 7:32 PM, Andy Zhou <az...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
