[ovs-dev] [PATCH 04/11] ofproto-dpif.at: Fix some races in megaflow tests

Joe Stringer joe at wand.net.nz
Tue Apr 8 02:01:39 UTC 2014


On 8 Apr 2014 12:56, "YAMAMOTO Takashi" <yamamoto at valinux.co.jp> wrote:

> > On 7 April 2014 20:12, YAMAMOTO Takashi <yamamoto at valinux.co.jp> wrote:
> >
> >> > I've been looking at some races in the megaflow testcases recently as
> >> well,
> >> > with a slightly different approach, could you a look at my patch
> below?
> >> >
> >> > http://openvswitch.org/pipermail/dev/2014-April/038536.html
> >>
> >> my patch inserts sleeps between two netdev-dummy/receive calls,
> >> where it's expected that the former packet inserts flow and the
> >> latter one hits it.  i don't think your patch helps this case.
> >>
> >> in general, your patch doesn't eliminate the need of "sleep 1"
> >> because it still have a race with dispatcher and upcall handler
> >> threads, does it?
> >
> >
> > I see, that patch changes what we are testing. If the correct set of
> flows
> > are installed at all, then the test passes. It does not check that one
> > packet causes flow installation, then one packet hits it. I think that
> for
> > the most part, the megaflow tests are checking that flows with the
> correct
> > masks are installed, and different packets do not cause flows to be
> > installed with incorrect masks. Correct me if I'm wrong, but I think that
> > grepping logs for flow installation messages should cover this case,
> right?
>
> grepping logs should be ok.  but i'm not sure if it's an improvement
> over the current method unless it eliminates races.
>
> let me explain my understanding.
> netdev-dummy/receive merely puts the packet on the queue.
> after involving some of threads, finally the upcall handler
> thread installs the flow and thus output the logs which you grep.
> (well, vlog_enable_async can make the situation even worse.)
> so you still need some "sleep" between netdev-dummy/receive and grep.
>

Thanks for your patience, I understand now :)

You're right, it doesn't cover such a case where the test script greps the
log before the upcall handler has a chance to run and create the log. I
replied with clarification on the other patch.

This patch appears to insert additional sleeps in between the packets being
sent, rather than between the packets being sent and calling the dump-flows
command. This is why I was curious if they were related.

If our race conditions are unrelated, then this looks fine by me. We can
keep thinking about a better solution, but in the mean time:

Acked-by: Joe Stringer <joe at wand.net.nz>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140408/ba7a9d2b/attachment-0005.html>


More information about the dev mailing list