On Thu, Dec 15, 2011 at 05:21:23PM -0800, Jesse Gross wrote: > 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.
Accurate stats might be a worthwhile improvement on their own. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
