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

[Andy Zhou]

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:
> >> >  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?
>
Just how it reads to me. O.K. I will remove it in the next patch

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

>
> >> > +               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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130614/0b66b946/attachment-0003.html>