Re: [PATCH] softirq queue is now list_head, eliminate bh_next]

Re: [PATCH] softirq queue is now list_head, eliminate bh_next]

[Luben Tuikov]

Patrick Mansfield wrote:
> On Fri, Mar 07, 2003 at 06:55:42PM -0500, Luben Tuikov wrote:
> 
>>Eliminated is the double loop in scsi_softirq() -- this is
>>better handled in do_softirq() and gives the system a ``breather''.
>>(There are pros and cons for either side and if you guys
>>think that it was better with the double loop, I'll change it and
>>resubmit the patch.)
> 
> 
> I think it is better to have one loop (per your patch) - in breather cases
> when we get multiple interrupts before we can service all of them the
> do_softirq() can wakeup ksoftirqd for us to run on.

Yes, exactly my reasoning, thus the single loop.

>>  static void scsi_softirq(struct softirq_action *h)
>>  {
>>-	int cpu = smp_processor_id();
>>-	struct softscsi_data *queue = &softscsi_data[cpu];
>>+	LIST_HEAD(local_q);
>>
>>-	while (queue->head) {
>>-		Scsi_Cmnd *SCpnt, *SCnext;
>>+	local_irq_disable();
>>+	list_splice_init(&done_q[smp_processor_id()], &local_q);
>>+	local_irq_enable();
>>+
>>+	while (!list_empty(&local_q)) {
> 
> 
> Why not list_for_each_safe rather than a while?

Well, it's just simpler, since each element is, for sure,
deleted in sequence.

IMO, the macro list_for_each_safe() is very helpful (worth) when
there's a conditional (if/etc) which *may* delete the element
of the list, but since we're deleting it for sure, list_empty()
I think is simpler, and it saves as a struct list_head *tmp.

Furthermore list_for_each_*() implies _traversing_ the list in
some way, all the while we're just _consuming_ from the front
of a local list...  Me and my formalisms... :-)

[[I've used both profusely elsewhere, and that particular construct
has become an idiom for me (for its purpose).]]
 
> 
>>+		} /* switch (command disposition) */
>>+	} /* while (local queue is not emtpy) */
>>+} /* end scsi_softirq() */
>>
> 
> 
> You should get rid of the comments after the }'s.

I just simulated what was in the code before so as to stay
as close to the ``median'' as possible.

But ok, I'll delete them and resubmit the patch.

The only comment I put after a closing brace of a function
definition is /* end of fn_name() */ so that one can see
which function ends if its definition (code) is only partly
shown in one's screen.
I hope that's ok with everyone?  If not, flame me now.

Patrick, thanks for your input.

-- 
Luben

Re: [PATCH] softirq queue is now list_head, eliminate bh_next]

[Christoph Hellwig]

On Sat, Mar 08, 2003 at 12:56:14PM -0500, Luben Tuikov wrote:
> The only comment I put after a closing brace of a function
> definition is /* end of fn_name() */ so that one can see
> which function ends if its definition (code) is only partly
> shown in one's screen.
> I hope that's ok with everyone?  If not, flame me now.

No need for a flame, but please remove the comment, there is a strong
tendency against this style of commenting in the core kernel code.

A related note from the CondingStyle document:

	Functions should be short and sweet, and do just one thing.  They should
	fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
	as we all know), and do one thing and do that well.

You're _code_ is actually fine regarding to that so you shouldn't need the
comments :)

Re: [PATCH] softirq queue is now list_head, eliminate bh_next]

[James Bottomley]

On Sat, 2003-03-08 at 11:56, Luben Tuikov wrote:
> I just simulated what was in the code before so as to stay
> as close to the ``median'' as possible.
> 
> But ok, I'll delete them and resubmit the patch.

Actually, if you're going to resubmit the patch, could you use per cpu
areas for the queues (rather than __cacheline_aligned)?  It saves space
(on some architectures) and avoids us having to worry about NR_CPUS.

Thanks,

James