Thanks Alex,
On 23 December 2014 at 19:04, Alex Wang <al...@nicira.com> wrote:
> When 'ovs-appctl revalidator/purge' is called, the main thread
> sweeps and destroys all ukeys and the associated datapath flows.
> If, at the same time, revalidators are dumping those flows from
> datapath, the ukey lookup of dumped flows could fail due to
> deletion by main thread. This race will also cause the mistaken
> issue of few warnings.
Perhaps we should include the actual warnings in the commit message?
The issue might be better described in terms of the revalidation
cycle, and the main thread modifying udpif state during a phase that
expects no modification. I intend to write up a separate patch to
document these phases to explain the threading model around udpif
access, which will hopefully make bugs like this more shallow.
> @@ -104,6 +104,8 @@ struct udpif {
>
> struct revalidator *revalidators; /* Flow revalidators. */
> size_t n_revalidators;
> + struct ovs_mutex revalidate_mutex; /* Must be taken during revalidation
> */
> + /* cycle or during purge. */
>
> struct latch exit_latch; /* Tells child threads to exit. */
>
Just floating an idea: Perhaps we could think of this as the flow
manager mutex. It prevents multiple threads from attempting to manage
(delete) the datapath flows at once. Whoever takes the mutex must
ensure that operations on the datapath are mirrored with operations to
the umaps, and vice versa.
> @@ -709,7 +713,7 @@ free_dupcall:
> }
>
> static void *
> -udpif_revalidator(void *arg)
> +udpif_revalidator(void *arg) OVS_NO_THREAD_SAFETY_ANALYSIS
> {
> /* Used by all revalidators. */
> struct revalidator *revalidator = arg;
It's a pity that we drop threadsafety analysis in this function to fix
a particular race condition, but I can't come up with a better way.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
