Re: [patch] RCU for low latency [2/2]

[Rusty Russell]

In message <20040108115051.GC5128@xxxxxxxxxx> you write:
> @@ -54,6 +54,11 @@ DEFINE_PER_CPU(struct rcu_data, rcu_data
>  /* Fake initialization required by compiler */
>  static DEFINE_PER_CPU(struct tasklet_struct, rcu_tasklet) = {NULL};
>  #define RCU_tasklet(cpu) (per_cpu(rcu_tasklet, cpu))
> +#ifdef CONFIG_PREEMPT
> +static int rcu_max_bh_callbacks = 256;
> +#else 
> +static int rcu_max_bh_callbacks = 0;
> +#endif
>
> +static inline int rcu_limiting_needed(int cpu)
> +{
> +	if (in_softirq() && RCU_krcud(cpu))
> +		return 1;
> +	return 0;
> +}
> +
>  /*
>   * Invoke the completed RCU callbacks. They are expected to be in
>   * a per-cpu list.
> @@ -87,13 +99,24 @@ static void rcu_do_batch(struct list_hea
>  {
>  	struct list_head *entry;
>  	struct rcu_head *head;
> +	int count = 0;
> +	int cpu = smp_processor_id();
> +	int limit = rcu_limiting_needed(cpu);
>  
>  	while (!list_empty(list)) {
>  		entry = list->next;
>  		list_del(entry);
>  		head = list_entry(entry, struct rcu_head, list);
>  		head->func(head->arg);
> +		count++;
> +		if (limit && count > rcu_max_bh_callbacks &&
> +			rq_has_rt_task(cpu)) {

Perhaps you should do this the other way:

static inline unsigned int max_rcu_at_once(int cpu)
{
	if (in_softirq() && RCU_krcud(cpu) && rq_has_rt_task(cpu))
		return rcu_max_bh_callbacks;
	return (unsigned int)-1;
}

....

	unsigned int count, limit = max_rcu_at_once(cpu);

	...
		if (++count > limit)

Ideally you'd create a new workqueue for this, or at the very least
use kthread primitives (once they're in -mm, hopefully soon).

> +static int start_krcud(int cpu)
> +{
> +	if (rcu_max_bh_callbacks) {
> +		if (kernel_thread(krcud, (void *)(long)cpu, CLONE_KERNEL) < 0) {
> +			printk("krcud for %i failed\n", cpu);
> +			return -1;

EPERM seems unusual here.  How about:

	int pid;
	pid = kernel_thread(krcud, (void *)(long)cpu, CLONE_KERNEL);
	if (pid < 0) {
		printk(printk("krcud for %i failed\n", cpu);
		return pid;
	}


> +__setup("rcubhlimit=", rcu_bh_limit_setup);

Please use module_param().  If it's a good name for a variable, it's a
good name for a parameter.  eg. just call the variable bhlimit, and
then the boot parameter will be called "rcu.bhlimit" which fits the
calling conventio we're trying to encourage.

Cheers,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/