Ben, Thanks for the review!
In working towards making the classifier use RCU instead of locking I noticed that we still need the priority value copied to the priority list itself. For example, when a rule is deleted that was the sole highest priority rule in the subtable, the subtable’s max_priority needs to be lowered and it’s position in the list needs to be updated. Since these are two separate operations, the reader’s could see the priority list in wrong order, and if the readers can’t trust the order being correct, there is no point maintaining the priority order in the first place. The solution I’m working in is to update the subtable’s max_priority immediately, but have the readers to ignore that value. The subtable list has it’s own copy of the subtable’s priority for the readers to use, and it is updated by swapping the whole table. I.e., we allocate a new table, sort it properly, and then RCU set it so that no reader ever sees a priority table that is out of order. I also think this is a good time to factor the priority vector code out of the classifier. I have prepared a patch for doing this, adding files lib/pvector.[ch] that implement an RCU priority vector that hold a priority and a void *, so it should be a generic non-intrusive priority vector. More later, Jarno On May 28, 2014, at 9:49 AM, Ben Pfaff <[email protected]> wrote: > On Thu, May 22, 2014 at 06:38:53PM -0700, Ethan Jackson wrote: >> From: Jarno Rajahalme <[email protected]> >> >> Do not cache the 'tag' and 'max_priority' in the subtable array. This >> makes later changes to classifier easier. >> >> Change CLS_SUBTABLES_FOR_EACH to iterate with a regular array index. >> >> Makes the 'cls_subtables*' functions to always leave the subtables >> array in a consistent state. This includes the new >> cls_subtables_insert() function and removal of the old >> cls_subtables_push_back() function. >> >> Finally, using qsort() instead of our hand rolled sort routines. This >> simplifies the code significantly. >> >> Signed-off-by: Jarno Rajahalme <[email protected]> >> Co-authored-by: Ethan Jackson <[email protected]> >> Signed-off-by: Ethan Jackson <[email protected]> >> --- >> >> Jarno wrote the bulk of this patch, but I folded in the switch to use the >> qsort() function. I was planning to write that on top of this, but it turned >> out to be much easier to fold it in. Someone other than either of us should >> probably review it. > > Some of the new functions in classifier.c are marked explicitly > inline. This is usually a bad idea because it suppresses warnings > about functions that are never called (and because it usually doesn't > help code generation). > > I don't see a reason to change the cls_subtables member 'count' from > size_t to int. (The previous versions of the patch did have a reason, > but it doesn't appear valid anymore.) Similarly, I would use size_t > for the index parameter to cls_subtables_remove(). > > This new comment in cls_subtables_insert() seems like a non sequitur: >> + /* Lowest priority subtables at the end. */ >> + cls->subtables.array[cls->subtables.count++] = a; >> + sort_subtables(&cls->subtables); >> } > > Now that CodingStyle allows variable declarations in 'for' statements, > CLS_SUBTABLES_FOR_EACH can be made easier to use. Instead of: > #define CLS_SUBTABLES_FOR_EACH(SUBTABLE, INDEX, SUBTABLES) \ > for ((INDEX) = 0; \ > (INDEX) < (SUBTABLES)->count \ > && ((SUBTABLE) = (SUBTABLES)->array[INDEX], true); \ > (INDEX)++) > one can write: > #define CLS_SUBTABLES_FOR_EACH(SUBTABLE, SUBTABLES) \ > for (size_t index_ = 0; \ > index_ < (SUBTABLES)->count \ > && ((SUBTABLE) = (SUBTABLES)->array[index_], true); \ > index_++) > eliminating the INDEX macro argument. > > cmp_subtable() could use this really simple way to get a return value > for qsort(), which has been my favorite for a long time: > return a > b ? -1 : a < b; > > Acked-by: Ben Pfaff <[email protected]> > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
