softirq queue is now list_head, eliminate bh_next]
Quoted 9 lines · Patrick Mansfield
Quoted 5 lines · Luben Tuikov
Eliminated is the double loop in
scsi_softirq()-- this is better handled indo_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.)
Quoted 3 lines
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.
Quoted 6 lines
Quoted 6 lines
static void
scsi_softirq(struct softirq_action *h){(quoted diff) +1
-2- int cpu = smp_processor_id(); - struct softscsi_data *queue = &softscsi_data[cpu]; + LIST_HEAD(local_q);
Quoted 10 lines
Review
Quoted 7 lines
(quoted diff) +5
-2- 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_saferather 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).]]
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +3
-0+ } /* 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.
Signature · 1 line
Luben
Quoted 5 lines · Luben Tuikov
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 :)
Quoted 4 lines · Luben Tuikov
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