[ovs-dev] [PATCH 2/2] datapath: Add loop detection for RT kernels.

Ben Pfaff blp at nicira.com
Mon Oct 25 17:01:05 UTC 2010


On Fri, Oct 22, 2010 at 04:34:14PM -0700, Jesse Gross wrote:
> Our normal loop detection is fairly lightweight but requires
> disabling preemption while packet processing takes place.  On
> RT kernels this isn't acceptable and interacts badly with spinlocks,
> so we can't use it.  This implements a form of pseudo thread local
> storage and uses that instead as it is capable of tracking a thread
> across CPU migrations.  It is more expensive than the preemption
> disabled version, so we continue to use that on non-RT kernels.
> 
> Signed-off-by: Jesse Gross <jesse at nicira.com>

I assume that we would revert this before submitting upstream?

Is realtime with NR_CPUS==1 an interesting special case?  We could just
use the non-RT implementation in that case, I think.

Softirqs are always threaded on RT, right?  So that means that we'd
always have a task_struct.  RT adds an "extra_flags" member to
task_struct but only uses two bits of it.  We could abuse other bits as
a counter.

Does anything actually guarantee that NR_CPUS is a power of 2?  Writing
#define TASK_HASH_BUCKETS NR_CPUS and then "& (TASK_HASH_BUCKETS - 1)"
means that we depend on such a guarantee.

I think that task_hash_lock should be static.

Is there some reason to kzalloc task_hash[] instead of declaring it as a
static array of NR_CPU members?  Maybe this is a kernel programming rule
that I don't know about.

The { and } for the "if" inside find_task() are not aligned properly.
The } should be one tab stop farther right.

It's too bad that two atomic ops (atomic_inc_not_zero(),
atomic_dec_and_test()) are needed for something thread-local.

I don't see any actual problems with this, so with the above comments:
Acked-by: Ben Pfaff <blp at nicira.com>




More information about the dev mailing list