On 06/09/14 at 03:26pm, Andy Zhou wrote:
> On Mon, Jun 9, 2014 at 2:58 PM, Thomas Graf <[email protected]> wrote:
> > On 06/06/14 at 02:37pm, Andy Zhou wrote:
> >> +static void tbl_mask_array_delete_mask(struct mask_array *ma,
> >> + const struct sw_flow_mask *mask)
> >> +{
> >> + int i = 0;
> >> +
> >> + /* Delete a mask pointer from the valid section.
> >> + *
> >> + * Also move the last entry in its place, so there is no
> >> + * whole in the valid section.
> >> + *
> >> + * Notice the last entry still points to the original mask.
> >> + *
> >> + * <Note>: there is a small race window that may cause a mask
> >> + * to be missed in a search. Imaging a core is
> >> + * walking through the array, passing the index of deleting mask.
> >> + * But before reaching the last entry, it is overwritten,
> >> + * by another core that is adding a new mask, now the last entry
> >> + * will point to the new mask. In this case, the moved up last
> >> + * entry can be missed by the core walking the mask array.
> >> + *
> >> + * In case this missed mask would have led to successful
> >> + * lookup, Hitting the race window could cause a packet to miss
> >> + * kernel flow cache, and be sent to the user space.
> >> + * </Note>
> >> + */
> >> + while (i < ma->count - 1) {
> >
> > I think this should be coded as a for (;;) loop instead of
> > incrementing `i` in the else branch.
> I agree for loop would be easier to read, but in this case, I need
> to reconsider the just reloaded entry, so "i" will only move up if
> no deletion happens. It is not clear to me how to structure it as a for loop.
I think you could make it work as you stop iterating upon deletion,
like this:
for (i = 0; i < ma->count - 1; i++) {
if (mask == ma->masks[i]) {
[...]
break;
}
}
This is also how other similar kernel code looks like.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
