Re: [RFC PATCH 6/6] Convert tasklets to work queues

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 2007-06-22 at 00:06 -0700, Daniel Walker wrote:

> > +void tasklet_schedule(struct tasklet_struct *t)
> > +{
> > +	BUG_ON(!ktaskletd_wq);
> > +	pr_debug("scheduling tasklet %s %p\n", t->n, t);
> 
> I'd change these pr_debug lines to "tasklet : scheduling %s %p\n" for
> readability ..

As Ingo suggested, the next round won't even have them.



> truct tasklet_struct, work);
> > +
> > +	if (unlikely(atomic_read(&t->count))) {
> > +		pr_debug("tasklet disabled %s %p\n", t->n, t);
> > +		set_bit(TASKLET_STATE_PENDING, &t->state);
> > +		smp_mb();
> > +		/* make sure we were not just enabled */
> > +		if (likely(atomic_read(&t->count)))
> > +			goto out;
> > +		clear_bit(TASKLET_STATE_PENDING, &t->state);
> 
> smp_mb__before_clear_bit ?

The smp_mb() is needed before the atomic_read. But since that atomic
read is in a conditional, no more barriers are needed if we continue.

Thanks go out to Oleg for pointing out the smp_mb was needed!


> > +
> > +void __init softirq_init(void)
> > +{
> > +}
> 
> ifdef's ?

Nah, ifdefs are ugly. Even uglier than stubbed functions. I guess I
could simply put it in the header as a define do {} while(0).

> 
> > +void init_tasklets(void)
> > +{
> > +	ktaskletd_wq = create_workqueue("tasklets");
> > +	BUG_ON(!ktaskletd_wq);
> > +}
> > +
> > +void takeover_tasklets(unsigned int cpu)
> > +{
> > +	pr_debug("Implement takeover tasklets??\n");
> > +}
> 
> hmm .. Looks like it's for migration of tasklets .. I take it your not
> sure that's handled ? Try cpu hotplug ..


Actually, I believe that the workqueues will handle it themselves, so I
don't need to do any special handling. Just another advantage of
converting tasklets into work queues.

> 
> > +void tasklet_init(struct tasklet_struct *t,
> > +		  void (*func)(unsigned long), unsigned long data)
> > +{
> > +	INIT_WORK(&t->work, work_tasklet_exec);
> > +	INIT_LIST_HEAD(&t->list);
> > +	t->state = 0;
> > +	atomic_set(&t->count, 0);
> > +	t->func = func;
> > +	t->data = data;
> > +	t->n = "anonymous";
> 
> Is this "anonymous" just used for debugging ? Is so you could fill it
> with a kallsyms lookup with the __builtin_return_address() ..

Yeah, they were started for debugging, but I also thought about making
some sort of interface for user land to see what was defined. So using
kallsyms might not look well. It's easy enough to find if an anonymous
tasklet gives me trouble.


-- Steve