On Fri, Apr 3, 2015 at 4:21 PM, Joe Stringer <joestrin...@nicira.com> wrote:
> Missed this the first time around for some reason. Minor comments inline.
>
> On 1 April 2015 at 22:14, Neil McKee <neil.mc...@inmon.com> wrote:
> > 1) We can use the concurrent hash-map of flows that the revalidator
> > threads are sharing. The upcall knows the 128-bit ufid for the flow, so
> > we can get directly to the compiled actions like this:
> >
> > struct udpif_key *ukey = ukey_lookup(updif, upcall->ufid);
> > if(ukey) {
> > struct nladdr *actions = ofpbuf_data(ukey->actions);
> > ....
> > }
> >
> > This seems to work most of the time, but the way the revalidator threads
> > are spinning and flushing seems to make ukey_lookup() fail sometimes and
> > return NULL. Is there a different way to query that would avoid this
> > problem? If this method was reliable we could probably eliminate all
> the
> > code that builds and stores the sFlow userdata-cookie, and we would
> have a
> > mechanism that would work not just for tunnels, but for MPLS and other
> > actions too.
>
> You could turn up the 'dpif' debug level and ensure that you are
> actually seeing the UFIDs you expect (and log an error with the UFID
> when lookup fails).
>
When I get a match it is the right flow. It's just that the lookup fails
about 5% of the time. Is that just because of the way the revalidator
threads work? What sort of hit-rate would you expect? Should I get all
the busier flows, and miss when the flow has been idle a few seconds?
Also, I'm not quite sure if I am looking it up with the right lock / rcu
protections. Can I really just call ukey_lookup(updif, upcall->ufid)
there without taking any other lock first? And when I get a ukey pointer
back from the lookup, presumably I have to then take the lock on
ukey->mutex before I look at the actions. Once I have that lock then I
can see that everything will be stable, but it seems like there might be a
race: what if the ukey is freed under my feed before I get the ukey->mutex
lock?
struct udpif_key *ukey = ukey_lookup(updif, upcall->ufid);
if(ukey) {
/* race here? */
ovs_mutex_lock(ukey->mutex);
struct nladdr *actions = ofpbuf_data(ukey->actions);
....
ovs_mutex_unlock(ukey->mutex);
}
Do I need to indicate that this is an RCU critical region somehow?
>
> I'm not sure that you can guarantee the UFID is 'correct' (ie, same as
> the installed flow) for the action upcall case currently. The UFID
> doesn't originate in the kernel, so for all upcalls the dpif generates
> it based on the flow key and passes it up to ofproto-dpif. If the flow
> mangles the packet before performing sample(userspace()), then won't
> the flow key be different from the original flow?
>
The sFlow sample action is always the first action. Does that increase
your confidence?
I don't really mind if I get the actions straight from the kernel datapath
or looked up some other way in userspace. It just needs to be fairly
reliable. From what I can tell so far it works well enough (kind of) to do
the lookup in user-space. Well enough to proceed with the sFlow tunnel
export, anyway. Perhaps later we'll hit on a better way to do the
lookup, or modify the kernel to pass up the actions, but from there on
the treatment will be the same.
Neil
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
