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

[Jesse Gross]

On Fri, Jun 14, 2013 at 8:43 AM, Andy Zhou <azhou at nicira.com> wrote:
> On Fri, Jun 14, 2013 at 7:38 AM, Jesse Gross <jesse at nicira.com> wrote:
>> On Thu, Jun 13, 2013 at 7:32 PM, Andy Zhou <azhou at nicira.com> wrote:
>> >> > +       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.
>
> If it helps, we can add unlikely(). But this is slow path code after all.

I agree that it doesn't really matter and I wouldn't bother adding an
unlikely. I was mostly pointing out that from a performance
perspective it's not worse. It doesn't really matter that much though.

>> >> > +               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.
>
> I see the problem now.  Calling ovs_sw_flow_mask_del_ref() or just unlink
> the mask object from the list when the flow is deleted when ovs lock is
> still held makes sense. But when should the mask be kfreed? Does it still
> require RCU callback?

It's not strictly necessary (you could piggyback on the flow RCU
callback). However, it's probably cleaner to keep it separate since
there is a second entry point (the mask list traversal on flow
lookup).