On Thu, Dec 15, 2011 at 4:56 PM, Ben Pfaff <b...@nicira.com> wrote: > On Thu, Dec 15, 2011 at 04:49:57PM -0800, Jesse Gross wrote: >> >> > Could we just use the existing spinlock in sw_flow plus the 'dead' >> >> > variable to ensure that no packets go through a deleted flow after >> >> > it's deleted? ??On the delete side, take the spinlock and set 'dead'. >> >> > On the fast path, take the spinlock and check 'dead' before executing >> >> > the actions, release the spinlock after executing the actions. ??We >> >> > already have to take the spinlock for every packet anyway to update >> >> > the stats, so it's not an additional cost. ??I doubt that there's any >> >> > parallel work for a given flow anyway (that would imply packet >> >> > reordering). ??We would have to avoid recursive locking somehow. >> >> >> >> Hmm, I think that would probably work. ??It's slightly convoluted but >> >> certainly much less so than the alternatives. ??How would you get >> >> recursive locking? >> > >> > Flow loops e.g. through patch ports was what came to mind. >> >> We don't execute any actions while the spin lock is held and trips >> through a patch port will appear as different flows, so I don't think >> that there is a possibility of recursive locking. I think you >> basically do: >> >> For flow processing: >> >> * Extract and lookup flow >> * Take spin lock >> * Check to see if flow is dead >> * Else update stats >> * Release spin lock >> * If dead, act like you didn't find the flow >> * Else execute actions >> >> For flow deletion: >> >> * Take spin lock >> * Read stats >> * Mark flow dead >> * Release spin lock > > My intention was to make "Release spin lock" the last step in both > cases, hence the possibility of recursion. Otherwise, this change > ensures that stats are correct but side effects such as queueing a > copy of a packet to userspace can still happen after userspace sees > the reply to its deletion.
I was designing for the accurate stats case, I suppose you would need to hold onto the lock for the duration of flow execution in order to handle packets going to userspace. I'm not sure how I feel about holding that spinlock for that whole time, including all the way through the rest of the network stack, although I suppose it's not too different from the TX queue lock on devices. We could handle recursion the same way they do, by checking to see if the current CPU owns the lock it is trying to acquire. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
