> On 8 Apr 2014 12:56, "YAMAMOTO Takashi" <[email protected]> wrote: > >> > On 7 April 2014 20:12, YAMAMOTO Takashi <[email protected]> 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 <[email protected]>
thanks. however, after another thought, i dropped this patch for now. while the additional sleep avoids a race, it makes the window for another race (the one you mentioned) larger. given that the default max_idle is 1500ms and the revalidation interval is 500ms, this addititional 1 second sleep seems more dangerous than i thought. as this "sleep 1" is to ensure threads have enough cpu slices to make a progress, probably it should be with time/stop. YAMAMOTO Takashi _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
