the attached (compressed) patch is the next iteration of the workqueue
abstraction. There are two major categories of changes:
- a much improved (i hope) core framework
- (almost) complete conversion of existing drivers to the new framework.
1) The framework improvements include:
- per-CPU queueing support.
on SMP there is a per-CPU worker thread (bound to its CPU) and per-CPU
work queues - this feature is completely transparent to workqueue-users.
keventd automatically uses this feature. XFS can now update to work-queues
and have the same per-CPU performance as it had with its per-CPU worker
threads.
- delayed work submission
there's a new queue_delayed_work(wq, work, delay) function and a new
schedule_delayed_work(work, delay) function. The later one is used to
correctly fix former tq_timer users. I've reverted those changes in 2.5.40
that changed tq_timer uses to schedule_work() - eg. in the case of
random.c or the tty flip queue it was definitely the wrong thing to do.
delayed work means a timer embedded in work_t. I considered using split
work_t and delayed_work_t types, but lots of code actively uses
task-queues in both delayed and non-delayed mode, so i went for the more
generic approach that allows both methods of work submission. Delayed
timers do not cause any other overhead in the normal submission path
otherwise.
- multithreaded run_workqueue() implementation
the run_workqueue() function can now be called from multiple contexts, and
a worker thread will only use up a single entryy - this property is used
by the flushing code, and can potentially be used in the future to extend
the number of per-CPU worker threads.
- more reliable flushing
there's now a 'pending work' counter, which is used to accurately detect
when the last work-function has finished execution. It's also used to
correctly flush against timed requests. I'm not convinced whether the old
keventd implementation got this detail right.
- i switched the arguments of the queueing function(s) per Jeff's
suggestion, it's more straightforward this way.
2) driver fixes.
i have converted almost every affected driver to the new framework. This
cleaned up tons of code. I also fixed a number of drivers that were still
using BHs (these drivers did not compile in 2.5.40).
while this means lots of changes, it might ease the QA decision whether to
put this patch into 2.5.
The pach converts roughly 80% of all tqueue-using code to workqueues - and
all the places that are not converted to workqueues yet are places that do
not compile in vanilla 2.5.40 anyway, due to unrelated changes. I've
converted a fair number of drivers that do not compile in 2.5.40, and i
think i've managed to convert every driver that compiles under 2.5.40.
i've tested the patch on 2.5.40, UP and SMP x86, boots & works just fine.
I've also attached maxi-config, which is a pretty large .config - this
config file compiles the kernel just fine. (it's too large to be booted
though.) No workqueue/taskqueue related warning messages or compile
errors.
Ingo
On Tue, 1 Oct 2002, Ingo Molnar wrote:
> 2) driver fixes.
>
> i have converted almost every affected driver to the new framework. This
> cleaned up tons of code. I also fixed a number of drivers that were still
> using BHs (these drivers did not compile in 2.5.40).
I'm possibly messing things up here, but doesn't it generally make more
sense to convert tq_immediate users to tasklets instead of work queues?
tq_immediate users do not need process context, and one use I'm familiar
with is basically doing bottom half interrupt processing, e.g. in lots of
places in the ISDN code. Introducing a context switch for no obvious gain
there seems rather pointless to me?
The same may be true for the tq_timer users as well?
--Kai
On Tue, 1 Oct 2002, Jeff Garzik wrote:
> I still think that schedule_work() should have void* cookie passed to it
> directly, instead of at INIT_WORK time [and possibly changing it by hand
> in the driver, immediately before schedule_work() is called]
i dont think this is a good idea, this pretty much forces the argument
passing upon every user of the interface - which argument would be put
into the worqueue-entry struct anyway.
the code behaves just right when only PREPARE_WORK() is used - the
completion code leaves the entry in a restartable state. A full
INIT_WORK() is only needed at init time. (or if DECLARE_WORK() was used
then no INIT_WORK() is needed.) And this is all intentional.
> For drivers that pass an interface pointer like struct net_device*,
> INIT_WORK-time, the current scheme is fine, but when the cookie
> fluctuates more, it makes a lot more sense to pass void* to
> schedule_work() itself.
these places should use PREPARE_WORK().
Ingo
On Tue, 1 Oct 2002, Ingo Molnar wrote:
>
> the attached (compressed) patch is the next iteration of the workqueue
> abstraction. There are two major categories of changes:
Pease don't introduce more typedefs. They only hide what the hell the
thing is, which is actively _bad_ for structures, since passing a
structure by value etc is something that should never be done, for
example.
The few saved characters of typing do not actually _buy_ you anything
else, and only obscures what the thing is.
Also, it's against the Linux coding standard, which does not like adding
magic single-letter suffixes to things - that also is the case for your
strange "_s" suffix for a structure (the real suffix is "_struct").
Remember: typing out something is not bad. It's _especially_ not bad if
the typing makes it more clear what the thing is.
I've done a global search-and-replace on the patch. The resulting patch is
actually _cleaner_, because it also matches more closely the old code
(which used "struct tq_struct"), so things like tabbed comment alignment
etc tend to be more correct (not always, but closer).
Linus
Hi Ingo,
> the attached (compressed) patch is the next iteration of the workqueue
> abstraction. There are two major categories of changes:
does not compile on 2.5.40 with XFS. Compiles clean w/o your patch.
(I cannot have a look into your maxi-config.bz2 because it is corrupt)
See:
make[2]: Entering directory `/usr/src/linux-2.5.40/fs/xfs'
gcc -Wp,-MD,./.xfs_rtalloc.o.d -D__KERNEL__ -I/usr/src/linux-2.5.40/include
-Wall -Wstrict-prototypes -Wno-trigraphs -O2 -fomit-frame-pointer
-fno-strict-aliasing -fno-common -pipe -mpreferred-stack-boundary=2
-march=i686 -I/usr/src/linux-2.5.40/arch/i386/mach-generic -nostdinc
-iwithprefix include -DMODULE -I. -funsigned-char
-DKBUILD_BASENAME=xfs_rtalloc
-c -o xfs_rtalloc.o xfs_rtalloc.c
In file included from linux/xfs_linux.h:64,
from xfs.h:54,
from xfs_rtalloc.c:37:
fs/xfs/pagebuf/page_buf.h:50: linux/tqueue.h: No such file or directory
In file included from linux/xfs_linux.h:64,
from xfs.h:54,
from xfs_rtalloc.c:37:
fs/xfs/pagebuf/page_buf.h:217: field `pb_iodone_sched' has incomplete type
In file included from xfs.h:106,
from xfs_rtalloc.c:37:
fs/xfs/xfs_log_priv.h:441: field `ic_write_sched' has incomplete type
fs/xfs/xfs_log.h:62: warning: `_lsn_cmp' defined but not used
fixdep: ./.xfs_rtalloc.o.d: No such file or directory
make[2]: *** [xfs_rtalloc.o] Error 2
make[2]: Leaving directory `/usr/src/linux-2.5.40/fs/xfs'
make[1]: *** [xfs] Error 2
make[1]: Leaving directory `/usr/src/linux-2.5.40/fs'
make: *** [fs] Error 2
--
Kind regards
Marc-Christian Petersen
http://sourceforge.net/projects/wolk
PGP/GnuPG Key: 1024D/569DE2E3DB441A16
Fingerprint: 3469 0CF8 CA7E 0042 7824 080A 569D E2E3 DB44 1A16
Key available at http://www.keyserver.net. Encrypted e-mail preferred.
Ingo,
Looking real good.
I still think that schedule_work() should have void* cookie passed to it
directly, instead of at INIT_WORK time [and possibly changing it by hand
in the driver, immediately before schedule_work() is called]
For drivers that pass an interface pointer like struct net_device*,
INIT_WORK-time, the current scheme is fine, but when the cookie
fluctuates more, it makes a lot more sense to pass void* to
schedule_work() itself.
Further, schedule_work(wq,data) is conceptually very close to
my_work_func(data) and makes the code easier to trace through: it
becomes more obvious what is the value of the my_work_func arg, at the
place in the code where schedule_work() is called. I see passing the
void* cookie as covering one common case, while adding void* arg to
schedule_work() would cover all cases...
[IMO the same argument can be applied to the existing timer API as well,
but timers are less often one-shot in kernel code, so it matter less...]
That said, I don't feel strongly about this, so can be convinced
otherwise fairly easily :) I would not complain if Linus applied your
patch as-is.
Jeff
On Tue, 1 Oct 2002, Linus Torvalds wrote:
> > the attached (compressed) patch is the next iteration of the workqueue
> > abstraction. There are two major categories of changes:
>
> Pease don't introduce more typedefs. They only hide what the hell the
> thing is, which is actively _bad_ for structures, since passing a
> structure by value etc is something that should never be done, for
> example.
>
> The few saved characters of typing do not actually _buy_ you anything
> else, and only obscures what the thing is.
>
> Also, it's against the Linux coding standard, which does not like adding
> magic single-letter suffixes to things - that also is the case for your
> strange "_s" suffix for a structure (the real suffix is "_struct").
yes i agree that it's bad in the current context of Linux, mainly because
the use of typedefs is inconsistent - the search-and-replace of work_t,
cpu_workqueue_t and workqueue_t is what needs to happen, i'm not arguing
about that - and the real discussion ends here.
-------------------------------------------------------------------------
but, at the danger of getting into another religious discussion.
Despite all the previous fuss about the problems of typedefs, i've never
had *any* problem with using typedefs in various code i wrote. It only
ever made things cleaner - to me. I had no problems with supposed
declaration limitations of typedefs or anything either. I in fact consider
it a feature that an unclean hiearchy of include files cannot be plastered
with typedef predeclarations.
what issue remains is purely the compactness and effectivity of the source
code representation. It confuses the human eye (at least mine) to see
'struct ' all over again. (In fact 'unsigned int' is confusing as well, so
i tend to use 'int' wherever possible safely.)
*If* the consistent convention were to use the _t postfix for complex
'derived' types, it would create more compact and more readable kernel
code. Why does it have to be:
static inline void __list_add(struct list_head *new,
struct list_head *prev,
struct list_head *next);
why not a simple:
static inline void __list_add(list_t *new, list_t prev, list_t *new);
perhaps it's because i dont use syntax highlighting, where the 'struct'
keyword sticks out and is not nearly as confusing as in an all-grey
display of the source code.
> Remember: typing out something is not bad. It's _especially_ not bad if
> the typing makes it more clear what the thing is.
I think writing out stuff makes sense only as long as it carries real,
important and unique information that triggers the proper association in
the human brain, without using up too much cognitive power (which is
needed for other stuff like writing code).
and in fact i believe that *not* writing out stuff that does nto matter is
just as important, to increase the signal-to-noise ratio of the visible
code area. I often find myself wondering about variable names and function
names, trying to make them shorter, without them losing any of their
expressive value.
and typing out 'struct ' all the time, i believe, often hides the real
information.
Real information such as a field _name_, or a type _name_, is the
essential stuff. All the rest, like paranthesis or commas are important
glue that needs to be as minimal and as expressive as possible - and it's
very rare that a 7-character construct carries any powerful meaning. In
fact i strongly believe that 'struct ' does not deserve the space it takes
- the fact that the type is 'complex' needs to be 'glued' upon the name,
it should not contaminate the name itself. Sure, we need to know what the
type is, but more so do we need to know *which* specific type it is.
I think everyone agrees that screen real estate (which is what the brain
also sees at once) must be used sparingly, and essential information must
be preserved. The most common abstractions need abbreviations (as long as
they do not limit understandability) - eg. a comma or paranthesis is an
essential tool - basically every other symbol is used up as well. I cant
see where the problem is with using _t postfixes to mark abstract
sub-types - everywhere.
i've done a quick experiment, every .h and .c file from the 2.5.40 kernel
in a single file:
-rw-rw-r-- 1 mingo mingo 133851995 Oct 1 21:55 all-struct.c
and the same file, but this time all 165636 occurances of 'struct '
replaced with '_t':
-rw-rw-r-- 1 mingo mingo 132819955 Oct 1 21:57 all-t.c
this shaves off almost 1% from the source code size. It might not look
significant, but i think it is a significant reduction in screen real
estate usage. For more complex pieces of code it can be more significant
than this average number - easily 5% or more.
sure, there is the problem of pte_t and kdev_t, which we want to deal with
as integer-alike basic types, but they are an order less important and
less frequent than complex types, so we might as well rename pte_t and use
another convention there.
and i found the _t convention especially useful in cases where there's
complex code, which wants to be split up into multiple inline functions.
And i believe once a function declaration becomes multiline it quickly
loses its efficiency.
> I've done a global search-and-replace on the patch. The resulting patch
> is actually _cleaner_, because it also matches more closely the old code
> (which used "struct tq_struct"), so things like tabbed comment alignment
> etc tend to be more correct (not always, but closer).
now what sense on earth does the duplication of 'struct' in 'struct
tq_struct' make? I very well know it's a 'struct' upon the first reading
of it.
Source code has precisely the same kind of very subtle, finegrained and
interwoven usability problems as desktop applications. I agree that
consistency is more important than having the absolute best rules, but i
do not buy 'struct tq_struct' over 'work_t'.
if we have to go with the 'struct' convention then rather 'struct work'
and 'struct workqueue' and 'struct cpu_workqueue'. (neither of them
collides with any other symbol in the existing namespace.)
This is perhaps one of the few areas where FreeBSD 'looks' a bit cleaner
than Linux. [at places where their code does not drown in #ifdefs that is
- everyone has their own set of problems.]
There _is_ a sort of Huffman code possible for source code as well (at
least for the common constructs), and one of the successes of C was that
it is 'concept-compact' above all. After all this is one reason that moved
people away from 'GOSUB B' to 'b();'.
C is still an inconsitent and limited language if compared with some of
the other highlevel languages, but it is compact and thus uses a minimum
of 'in-brain instruction cache'. I can see a parallel between the success
of C and the success of the x86 instruction set here ...
but, i'm trying to argue about taste, which is admittedly not an exact
science. :)
btw., usability was one of the main reasons why i thought alot about
changing to the word 'task' to 'work' or not. The 'wq' abbreviation has
the problem that it's kindof already used for the 'waitqueue' concept.
But the 'task' word is already taken as well, and i thought it's more
correct to have the type name right than to have an absolutely unique
typical variable name. (But it's not a clear winner so i might be wrong.)
Ingo
In article <[email protected]>,
Linus Torvalds <[email protected]> wrote:
>
>Pease don't introduce more typedefs. They only hide what the hell the
>thing is, which is actively _bad_ for structures, since passing a
>structure by value etc is something that should never be done, for
>example.
Btw, just to avoid counter-examples: Linux does use structures and
typedefs occasionally to hide and force compiler typechecking on small
structures on purpose. We have a few places where we do things like
typedef struct {
unsigned int value;
} atomic_t;
(and similar things for the page table entries etc).
This is done because the things are often really regular scalars, but we
use the structure as a strict type checking mechanism. In this case,
using a typedef is fine, because we don't actually ever want to _access_
it as a structure, and the typedef provices exactly the kind of
information hiding that we need.
But type hiding for a real structure just doesn't make sense, since we
use it as a true structure, and hiding information just makes it harder
to see.
Linus
On Tue, Oct 01, 2002 at 08:52:27PM +0200, Marc-Christian Petersen wrote:
> does not compile on 2.5.40 with XFS. Compiles clean w/o your patch.
> (I cannot have a look into your maxi-config.bz2 because it is corrupt)
I posted the 2.5.40 fix on this list some time ago.
The top of tree fix (different) is below:
diff -uNr -p linux-2.5-xfs/fs/xfs/pagebuf/page_buf.c /home/hch/linux/fs/xfs/pagebuf/page_buf.c
--- linux-2.5-xfs/fs/xfs/pagebuf/page_buf.c Tue Oct 1 22:26:22 2002
+++ /home/hch/linux/fs/xfs/pagebuf/page_buf.c Tue Oct 1 22:11:17 2002
@@ -179,6 +179,11 @@ pagebuf_param_t pb_params = {{ HZ, 15 *
struct pbstats pbstats;
/*
+ * Queue for delayed I/O completion.
+ */
+struct workqueue_struct *pagebuf_workqueue;
+
+/*
* Pagebuf allocation / freeing.
*/
@@ -1167,7 +1172,7 @@ _pagebuf_wait_unpin(
* present, will be called as a side-effect.
*/
void
-pagebuf_iodone_sched(
+pagebuf_iodone_work(
void *v)
{
page_buf_t *pb = (page_buf_t *)v;
@@ -1196,11 +1201,8 @@ pagebuf_iodone(
PB_TRACE(pb, PB_TRACE_REC(done), pb->pb_iodone);
if ((pb->pb_iodone) || (pb->pb_flags & PBF_ASYNC)) {
- INIT_TQUEUE(&pb->pb_iodone_sched,
- pagebuf_iodone_sched, (void *)pb);
-
- schedule_task(&pb->pb_iodone_sched);
-
+ INIT_WORK(&pb->pb_iodone_work, pagebuf_iodone_work, pb);
+ queue_work(pagebuf_workqueue, &pb->pb_iodone_work);
} else {
up(&pb->pb_iodonesema);
}
@@ -1854,6 +1856,10 @@ pagebuf_daemon_start(void)
kernel_thread(pagebuf_daemon, (void *)pb_daemon,
CLONE_FS|CLONE_FILES|CLONE_VM);
+
+ pagebuf_workqueue = create_workqueue("pagebuf");
+ if (!pagebuf_workqueue)
+ return -1;
}
return 0;
}
@@ -1867,6 +1873,9 @@ STATIC void
pagebuf_daemon_stop(void)
{
if (pb_daemon) {
+ flush_workqueue(pagebuf_workqueue);
+ destroy_workqueue(pagebuf_workqueue);
+
pb_daemon->active = 0;
pb_daemon->io_active = 0;
diff -uNr -p linux-2.5-xfs/fs/xfs/pagebuf/page_buf.h /home/hch/linux/fs/xfs/pagebuf/page_buf.h
--- linux-2.5-xfs/fs/xfs/pagebuf/page_buf.h Tue Oct 1 22:26:22 2002
+++ /home/hch/linux/fs/xfs/pagebuf/page_buf.h Tue Oct 1 22:11:38 2002
@@ -47,7 +47,7 @@
#include <linux/fs.h>
#include <linux/buffer_head.h>
#include <linux/uio.h>
-#include <linux/tqueue.h>
+#include <linux/workqueue.h>
enum xfs_buffer_state { BH_Delay = BH_PrivateStart };
BUFFER_FNS(Delay, delay);
@@ -214,7 +214,7 @@ typedef struct page_buf_s {
size_t pb_buffer_length; /* size of buffer in bytes */
size_t pb_count_desired; /* desired transfer size */
void *pb_addr; /* virtual address of buffer */
- struct tq_struct pb_iodone_sched;
+ struct work_struct pb_iodone_work;
page_buf_iodone_t pb_iodone; /* I/O completion function */
page_buf_relse_t pb_relse; /* releasing function */
page_buf_bdstrat_t pb_strat; /* pre-write function */
@@ -394,5 +394,7 @@ static __inline__ int __pagebuf_ioreques
return pb->pb_strat(pb);
return pagebuf_iorequest(pb);
}
+
+extern struct workqueue_struct *pagebuf_workqueue;
#endif /* __PAGE_BUF_H__ */
diff -uNr -p linux-2.5-xfs/fs/xfs/xfs_log.c /home/hch/linux/fs/xfs/xfs_log.c
--- linux-2.5-xfs/fs/xfs/xfs_log.c Thu Sep 26 21:26:31 2002
+++ /home/hch/linux/fs/xfs/xfs_log.c Tue Oct 1 22:00:27 2002
@@ -2714,7 +2714,7 @@ xlog_state_put_ticket(xlog_t *log,
LOG_UNLOCK(log, s);
} /* xlog_state_put_ticket */
-void xlog_sync_sched(
+void xlog_sync_work(
void *v)
{
xlog_in_core_t *iclog = (xlog_in_core_t *)v;
@@ -2773,13 +2773,12 @@ xlog_state_release_iclog(xlog_t *log,
* flags after this point.
*/
if (sync) {
- INIT_TQUEUE(&iclog->ic_write_sched,
- xlog_sync_sched, (void *) iclog);
+ INIT_WORK(&iclog->ic_write_work, xlog_sync_work, iclog);
switch (xlog_mode) {
case 0:
return xlog_sync(log, iclog, 0);
case 1:
- pagebuf_queue_task(&iclog->ic_write_sched);
+ queue_work(pagebuf_workqueue, &iclog->ic_write_work);
}
}
return (0);
diff -uNr -p linux-2.5-xfs/fs/xfs/xfs_log_priv.h /home/hch/linux/fs/xfs/xfs_log_priv.h
--- linux-2.5-xfs/fs/xfs/xfs_log_priv.h Thu Sep 26 21:26:31 2002
+++ /home/hch/linux/fs/xfs/xfs_log_priv.h Tue Oct 1 22:09:40 2002
@@ -438,7 +438,7 @@ typedef struct xlog_iclog_fields {
int ic_bwritecnt;
ushort_t ic_state;
char *ic_datap; /* pointer to iclog data */
- struct tq_struct ic_write_sched;
+ struct work_struct ic_write_work;
} xlog_iclog_fields_t;
typedef struct xlog_in_core2 {
@@ -458,7 +458,7 @@ typedef struct xlog_in_core {
* Defines to save our code from this glop.
*/
#define ic_forcesema hic_fields.ic_forcesema
-#define ic_write_sched hic_fields.ic_write_sched
+#define ic_write_work hic_fields.ic_write_work
#define ic_next hic_fields.ic_next
#define ic_prev hic_fields.ic_prev
#define ic_bp hic_fields.ic_bp
On Tue, 1 Oct 2002, Ingo Molnar wrote:
>
> Despite all the previous fuss about the problems of typedefs, i've never
> had *any* problem with using typedefs in various code i wrote.
Big things should have big names. That's why "u8" is u8, because it's not
just physically small, it also has very little semantics associated with
it.
I want those variable declarations to stand out, and make people
understand that this is not just a variable, it's a structure, and it may
be taking up a noticeable amount of space on the stack, for example.
That's the main issue for me. I don't personally care so much about trying
to avoid dependencies in the header files that can also be problematic.
That's probably partly because I use fast enough machines that parsing
them a few extra times doesn't much bother me, and circular requirements
tend to be rare enough not to bother me unduly.
So the thing is a big red warning sign that you're now using a complex
data structure, and you should be aware of the semantics that go with it.
Linus
At the risk of being on-topic <g>, would it be reasonable to request a
2.4 version of this patch, that can be used in driver-compat code and
vendor kernels? i.e. something that makes the workqueue API work in
2.4, no more, no less.
Jeff
On Tue, 1 Oct 2002, Linus Torvalds wrote:
> > Despite all the previous fuss about the problems of typedefs, i've never
> > had *any* problem with using typedefs in various code i wrote.
>
> Big things should have big names. That's why "u8" is u8, because it's
> not just physically small, it also has very little semantics associated
> with it.
>
> I want those variable declarations to stand out, and make people
> understand that this is not just a variable, it's a structure, and it
> may be taking up a noticeable amount of space on the stack, for example.
then lets at least have 'struct work' and 'struct work_queue' instead of
'struct work_struct', 'struct work_queue_struct'. "struct work" is already
twice as large as "work_t".
[ and eg. for 'struct list_head' i dont think the struct discrimination is
justified: the type takes up 8 bytes, twice as much as 'int', but the name
takes up 4 times as much space :-) ]
i dont think i've encountered much kernel code that tried to pass
structures along by value without a good reason - OTOH complex and
inefficient function interfaces outweigh these instances, by far. And
there's way too much code that has two screens full of local variable
declarations, where in the middle a 3K big array gets easily lost to the
eye. struct pre and postfix does not help much there.
And structure pointers are almost as simple to pass around and handle as
the basic types declared on the stack - and that is their main use. Ease
of understanding is i think by far the most important aspect of source
code - abuse and mistaken use of constructs is always possible, no matter
how long the name is.
and yet another thing - good complex structures are the ones that have not
only a compact name and compact usage, but also a compact meaning. Eg. the
work struct carries the following meaning: "call ->func(data) in process
context, at time X.". And intuition calls for a compact name for something
as compact as 'struct list_head'. For the case of task_t it's a different
matter - but nobody tries to use task_t not as a pointer anyway. Is there
really a category of structures that are commonly abused and put on the
stack, which would not happen that frequently via the 'struct *_struct'
convention?
Ingo
On Tue, 1 Oct 2002, Jeff Garzik wrote:
> At the risk of being on-topic <g>, would it be reasonable to request a
> 2.4 version of this patch, that can be used in driver-compat code and
> vendor kernels? i.e. something that makes the workqueue API work in
> 2.4, no more, no less.
i've concentrated all the code into kernel/workqueue.c, and all the
interfaces into include/linux/workqueue.h - so you can simply copy those
into the 2.4 tree and it should just work. There should be no namespace
collision either. A O(1) scheduler tree is needed, due to
set_cpus_allowed(), but there is no other dependency i think.
Ingo
On Tue, 1 Oct 2002, Kai Germaschewski wrote:
> I'm possibly messing things up here, but doesn't it generally make more
> sense to convert tq_immediate users to tasklets instead of work queues?
>
> tq_immediate users do not need process context, and one use I'm familiar
> with is basically doing bottom half interrupt processing, e.g. in lots
> of places in the ISDN code. Introducing a context switch for no obvious
> gain there seems rather pointless to me?
>
> The same may be true for the tq_timer users as well?
the main reason was that it was easier to convert everything (even old-BH
style code) that did deferred processing to workqueues than to tasklets -
since a fair chunk of deferred processing needs process context. Another
reason is that generally it's easier to handle overload situations if the
work is done in process contexts. But i agree, for things where it really
matters performance-wise, introducing a tasklet should be the next step.
Ingo
>>>>> "Ingo" == Ingo Molnar <[email protected]> writes:
Ingo> but, at the danger of getting into another religious discussion.
I don't want to get into flame land, but I think there's a couple of
important points.
Ingo> Despite all the previous fuss about the problems of typedefs,
Ingo> i've never had *any* problem with using typedefs in various code
Ingo> i wrote. It only ever made things cleaner - to me. I had no
Ingo> problems with supposed declaration limitations of typedefs or
Ingo> anything either. I in fact consider it a feature that an unclean
Ingo> hiearchy of include files cannot be plastered with typedef
Ingo> predeclarations.
The point here is that it probably makes the code easier for you to
read, but it makes it harder for a lot of other people since it's
inconsistent with the standard. When analyzing someone else's code and
having to go through a pile of typedef's to figure whether you are
dealing with structs or just renamed integer types is a major and
unnecessary pain.
Ingo> what issue remains is purely the compactness and effectivity of
Ingo> the source code representation. It confuses the human eye (at
Ingo> least mine) to see 'struct ' all over again. (In fact 'unsigned
Ingo> int' is confusing as well, so i tend to use 'int' wherever
Ingo> possible safely.)
I guess thats a matter of taste, when I write code I want to know what
I deal with. Same reason I find C++'s operator overloading to be
absolutely sickening.
Ingo> I think writing out stuff makes sense only as long as it carries
Ingo> real, important and unique information that triggers the proper
Ingo> association in the human brain, without using up too much
Ingo> cognitive power (which is needed for other stuff like writing
Ingo> code).
I will claim here that the fact something is a struct is very
important information. When someone else is to use the type, it's
important they know the actual cost of writing thigs like *a = *b. I
have seen this done way too many times, not just in C.
[snip]
Ingo> Sure, we need to know what the type is, but more so do we need
Ingo> to know *which* specific type it is.
foo_t doesn't tell you anything there either, foo_list_t says a bit
more, but still nothing about the actual cost of copying the type
around.
Ingo> i've done a quick experiment, every .h and .c file from the
Ingo> 2.5.40 kernel in a single file:
Ingo> -rw-rw-r-- 1 mingo mingo 133851995 Oct 1 21:55 all-struct.c
Ingo> and the same file, but this time all 165636 occurances of
Ingo> 'struct ' replaced with '_t':
Ingo> -rw-rw-r-- 1 mingo mingo 132819955 Oct 1 21:57 all-t.c
I haven't done this experient on the whole kernel, but from what I
have seen from many drivers, then you are going to gain a lot more by
replacing all whitespate indentations with tabs. In certain extreme cases
I have seen it reduce file siszes by more than 10%.
Ingo> if we have to go with the 'struct' convention then rather
Ingo> 'struct work' and 'struct workqueue' and 'struct
Ingo> cpu_workqueue'. (neither of them collides with any other symbol
Ingo> in the existing namespace.)
This would be a useful thing to clean up, or we should start naming
things foo_int as well ;-)
Ingo> but, i'm trying to argue about taste, which is admittedly not an
Ingo> exact science. :)
Yup, we agree on that, however I think a key issue here is that having
a common standard benefits a lot in terms of maintainability when we
have a multi developer project like this.
Anyway, just my $0.02.
Cheers,
Jes
On Wed, 2 Oct 2002, Christoph Hellwig wrote:
> On Tue, Oct 01, 2002 at 06:24:50PM +0200, Ingo Molnar wrote:
> >
> > the attached (compressed) patch is the next iteration of the workqueue
> > abstraction. There are two major categories of changes:
>
> What about forcing a flush in destory_workqueue?
yes, most definitely - this was done in yesterday's patch but forgot it in
today's.
Ingo
On Tue, Oct 01, 2002 at 06:24:50PM +0200, Ingo Molnar wrote:
>
> the attached (compressed) patch is the next iteration of the workqueue
> abstraction. There are two major categories of changes:
What about forcing a flush in destory_workqueue?
--- 1.1/kernel/workqueue.c Tue Oct 1 21:17:25 2002
+++ edited/kernel/workqueue.c Tue Oct 1 23:04:46 2002
@@ -317,6 +317,8 @@
struct cpu_workqueue_struct *cwq;
int cpu;
+ flush_workqueue(wq);
+
for (cpu = 0; cpu < NR_CPUS; cpu++) {
if (!cpu_online(cpu))
continue;
Ingo Molnar <[email protected]> writes:
> the attached (compressed) patch is the next iteration of the workqueue
> abstraction. There are two major categories of changes:
>
> - a much improved (i hope) core framework
>
> - (almost) complete conversion of existing drivers to the new framework.
>
> 1) The framework improvements include:
>
> - per-CPU queueing support.
>
> on SMP there is a per-CPU worker thread (bound to its CPU) and per-CPU
> work queues - this feature is completely transparent to workqueue-users.
> keventd automatically uses this feature. XFS can now update to work-queues
> and have the same per-CPU performance as it had with its per-CPU worker
> threads.
Hi Ingo,
I read through your patch and I think it looks great. I'm one of the
ieee1394 maintainers, and we also a have a worker thread mechanism in
the ieee1394 subsystem, which could (and will, I'm going to look into
this) be replaced by your work queue stuff. We use the thread for
reading configuration information from ieee1394 devices. This work
isn't very performance critical, but the reason we dont just use
keventd is that we don't want to stall keventd while reading this
information. So, my point is, that in this case (I'm sure there are
more situations like this, usb has a similar worker thread, khubd),
the per-cpu worker thread is overkill, and it would be sufficient with
just one thread, running on all cpus. So maybe this could be an
option to create_workqueue()? Either create a cpu-bound thread for
each cpu, or create one thread that can run on all cpus.
Another minor comment: why do you kmalloc() the workqueue_t? Wouldn't
it be more flexible to allow the user to provide a pointer to a
pre-allocated workqueue_t structure, e.g.:
static workqueue_t aio_wq;
[...]
create_workqueue(&aio_wq, "aio");
Kristian
On 1 Oct 2002, Jes Sorensen wrote:
> The point here is that it probably makes the code easier for you to
> read, but it makes it harder for a lot of other people since it's
> inconsistent with the standard. [...]
i began the whole line of argument with this sentence:
> *If* the consistent convention were to use the _t postfix for complex
> 'derived' types, it would create more compact and more readable kernel
> code.
furthermore:
> I agree that consistency is more important than having the absolute best
> rules, [...]
so i think we are in violent agreement regarding consistency.
Ingo
Ingo Molnar <[email protected]> writes:
> then lets at least have 'struct work' and 'struct work_queue' instead of
> 'struct work_struct', 'struct work_queue_struct'. "struct work" is already
> twice as large as "work_t".
I've always wondered about that -- why _does_ linux append `_struct' to
some structure names? It seems completely redundant (and ugly).
Thanks,
-Miles
--
The secret to creativity is knowing how to hide your sources.
--Albert Einstein
Hello!
On Tue, Oct 01, 2002 at 06:24:50PM +0200, Ingo Molnar wrote:
> The pach converts roughly 80% of all tqueue-using code to workqueues - and
> all the places that are not converted to workqueues yet are places that do
> not compile in vanilla 2.5.40 anyway, due to unrelated changes. I've
> converted a fair number of drivers that do not compile in 2.5.40, and i
> think i've managed to convert every driver that compiles under 2.5.40.
Here are corresponding changes to UML code (that were not done by Ingo just
because this code cannot be compiled due to Makefile bug).
This patch is required to make UML compilable again in bk-current (after you
fix the Makefile, of course).
I tested the patch and UML works fine for me.
Bye,
Oleg
===== arch/um/drivers/chan_kern.c 1.2 vs edited =====
--- 1.2/arch/um/drivers/chan_kern.c Mon Sep 30 11:59:19 2002
+++ edited/arch/um/drivers/chan_kern.c Wed Oct 2 12:08:57 2002
@@ -395,7 +395,7 @@
return(-1);
}
-void chan_interrupt(struct list_head *chans, struct tq_struct *task,
+void chan_interrupt(struct list_head *chans, struct work_struct *task,
struct tty_struct *tty, int irq, void *dev)
{
struct list_head *ele, *next;
@@ -409,7 +409,7 @@
do {
if((tty != NULL) &&
(tty->flip.count >= TTY_FLIPBUF_SIZE)){
- schedule_task(task);
+ schedule_work(task);
goto out;
}
err = chan->ops->read(chan->fd, &c, chan->data);
===== arch/um/drivers/line.c 1.1 vs edited =====
--- 1.1/arch/um/drivers/line.c Fri Sep 6 21:29:28 2002
+++ edited/arch/um/drivers/line.c Wed Oct 2 12:02:14 2002
@@ -215,7 +215,7 @@
if(err) goto out;
}
enable_chan(&line->chan_list, line);
- INIT_TQUEUE(&line->task, line_timer_cb, line);
+ INIT_WORK(&line->task, line_timer_cb, line);
}
if(!line->sigio){
===== arch/um/drivers/mconsole_kern.c 1.1 vs edited =====
--- 1.1/arch/um/drivers/mconsole_kern.c Fri Sep 6 21:29:28 2002
+++ edited/arch/um/drivers/mconsole_kern.c Wed Oct 2 12:06:54 2002
@@ -13,7 +13,7 @@
#include "linux/ctype.h"
#include "linux/interrupt.h"
#include "linux/sysrq.h"
-#include "linux/tqueue.h"
+#include "linux/workqueue.h"
#include "linux/module.h"
#include "linux/proc_fs.h"
#include "asm/irq.h"
@@ -42,7 +42,7 @@
LIST_HEAD(mc_requests);
-void mc_task_proc(void *unused)
+void mc_work_proc(void *unused)
{
struct mconsole_entry *req;
unsigned long flags;
@@ -60,10 +60,7 @@
} while(!done);
}
-struct tq_struct mconsole_task = {
- routine: mc_task_proc,
- data: NULL
-};
+DECLARE_WORK(mconsole_work, mc_work_proc, NULL);
void mconsole_interrupt(int irq, void *dev_id, struct pt_regs *regs)
{
@@ -84,7 +81,7 @@
}
}
}
- if(!list_empty(&mc_requests)) schedule_task(&mconsole_task);
+ if(!list_empty(&mc_requests)) schedule_work(&mconsole_work);
reactivate_fd(fd, MCONSOLE_IRQ);
}
===== arch/um/include/chan_kern.h 1.1 vs edited =====
--- 1.1/arch/um/include/chan_kern.h Fri Sep 6 21:29:28 2002
+++ edited/arch/um/include/chan_kern.h Wed Oct 2 12:07:57 2002
@@ -22,7 +22,7 @@
void *data;
};
-extern void chan_interrupt(struct list_head *chans, struct tq_struct *task,
+extern void chan_interrupt(struct list_head *chans, struct work_struct *task,
struct tty_struct *tty, int irq, void *dev);
extern int parse_chan_pair(char *str, struct list_head *chans, int pri,
int device, struct chan_opts *opts);
===== arch/um/include/line.h 1.1 vs edited =====
--- 1.1/arch/um/include/line.h Fri Sep 6 21:29:28 2002
+++ edited/arch/um/include/line.h Wed Oct 2 12:01:27 2002
@@ -7,7 +7,7 @@
#define __LINE_H__
#include "linux/list.h"
-#include "linux/tqueue.h"
+#include "linux/workqueue.h"
#include "linux/tty.h"
#include "asm/semaphore.h"
#include "chan_user.h"
@@ -39,7 +39,7 @@
char *head;
char *tail;
int sigio;
- struct tq_struct task;
+ struct work_struct task;
struct line_driver *driver;
int have_irq;
};
On Tue, 1 Oct 2002, Ingo Molnar wrote:
| i dont think i've encountered much kernel code that tried to pass
| structures along by value without a good reason - OTOH complex and
| inefficient function interfaces outweigh these instances, by far. And
| there's way too much code that has two screens full of local variable
| declarations, where in the middle a 3K big array gets easily lost to the
| eye. struct pre and postfix does not help much there.
Sounds like a good reason to have a gcc flag, or more likely a
Stanford checker or smatch checker for structs (or large typedefs :)
as return values.
| And structure pointers are almost as simple to pass around and handle as
| the basic types declared on the stack - and that is their main use. Ease
| of understanding is i think by far the most important aspect of source
| code - abuse and mistaken use of constructs is always possible, no matter
| how long the name is.
--
~Randy
On Tue, Oct 01, 2002 at 10:29:02PM +0200, Ingo Molnar wrote:
> On Tue, 1 Oct 2002, Linus Torvalds wrote:
> > Pease don't introduce more typedefs. They only hide what the hell the
> > thing is, which is actively _bad_ for structures, since passing a
[...]
> Despite all the previous fuss about the problems of typedefs, i've never
> had *any* problem with using typedefs in various code i wrote. It only
> ever made things cleaner - to me.
Hi Ingo,
I follow your reasoning, but one thing I don't follow -
+typedef struct work_s {
+ unsigned long pending;
+ struct list_head entry;
+ void (*func)(void *);
+ void *data;
+ void *wq_data;
+ timer_t timer;
+} work_t;
- why two names for the structure ("struct work_s" and "work_t")?
Either convention will work, but by declaring the structure twice it only
encourages users to employ their own favorite - thus defeating the purpose
of a convention.
Just curious,
-Kevin
--
------------------------------------------------------------------------
| Kevin O'Connor "BTW, IMHO we need a FAQ for |
| [email protected] 'IMHO', 'FAQ', 'BTW', etc. !" |
------------------------------------------------------------------------
On 1 Oct 2002, Kristian Hogsberg wrote:
> I read through your patch and I think it looks great. I'm one of the
> ieee1394 maintainers, and we also a have a worker thread mechanism in
> the ieee1394 subsystem, which could (and will, I'm going to look into
> this) be replaced by your work queue stuff. We use the thread for
> reading configuration information from ieee1394 devices. This work
> isn't very performance critical, but the reason we dont just use keventd
> is that we don't want to stall keventd while reading this information.
> So, my point is, that in this case (I'm sure there are more situations
> like this, usb has a similar worker thread, khubd), the per-cpu worker
> thread is overkill, and it would be sufficient with just one thread,
> running on all cpus. So maybe this could be an option to
> create_workqueue()? Either create a cpu-bound thread for each cpu, or
> create one thread that can run on all cpus.
i understand your point, and i really tried to get this problem solved
prior sending the multiple-workers patch, but it's not really doable
without major kludges. Is it really a problem on SMP boxes to have a few
more kernel threads? Those boxes are supposed to have enough RAM.
the only other way would be to introduce 1) runtime overhead [this sucks]
or 2) to split the API into per-CPU and global ones. [this isnt too good
either i think.]
there is enough flexibility internally - eg. we can in the future do
better load-balancing (if the XFS people will ever notice any problems in
this area), because right now the load-balancing is the simplest and
fastest variant: purely per-CPU. [in theory we could look at other CPU's
worker queues and queue there if they are empty or much shorter than this
CPU's worker queue.]
I also kept open the possibility of introducing multiple worker threads
per CPU in the future. But having a single-threaded an per-CPU behavior in
a single API looks quite hard.
we could perhaps do the following, add a single branch to the queue_work()
fastpath:
if (!cwq->thread)
cwq = wq->cpu_wq;
and the single-thread queue variant would thus fall back to using a single
queue only. Plus some sort of new variant could be used:
struct workqueue_struct *create_single_workqueue(char *name);
[or a 'flags' argument to create_workqueue();]
The runtime thing looks slightly ugly, but i think it's acceptable. Has
anyone any better idea?
> Another minor comment: why do you kmalloc() the workqueue_t? Wouldn't
> it be more flexible to allow the user to provide a pointer to a
> pre-allocated workqueue_t structure, e.g.:
>
> static workqueue_t aio_wq;
>
> [...]
>
> create_workqueue(&aio_wq, "aio");
yes, but every creation/destruction use of workqueues is in some sort of
init/shutdown very-slow-path, so efficiency is not a factor. But clarity
of the code is a factor, and in the dynamic allocation case it's much
cleaner to have this:
wq = create_workqueue("worker");
if (!wq)
goto error;
than:
wq = kmalloc(sizeof(*wq), GFP_KERNEL);
if (!wq)
goto error;
if (create_workqueue(wq, "worker")) {
kfree(w);
goto error;
}
[and we might even switch to a workqueue SLAB cache internally anytime.]
Ingo
Ingo Molnar <[email protected]> writes:
[...]
> > create_workqueue()? Either create a cpu-bound thread for each cpu, or
> > create one thread that can run on all cpus.
>
> i understand your point, and i really tried to get this problem solved
> prior sending the multiple-workers patch, but it's not really doable
> without major kludges. Is it really a problem on SMP boxes to have a few
> more kernel threads? Those boxes are supposed to have enough RAM.
>
> the only other way would be to introduce 1) runtime overhead [this sucks]
> or 2) to split the API into per-CPU and global ones. [this isnt too good
> either i think.]
>
> there is enough flexibility internally - eg. we can in the future do
> better load-balancing (if the XFS people will ever notice any problems in
> this area), because right now the load-balancing is the simplest and
> fastest variant: purely per-CPU. [in theory we could look at other CPU's
> worker queues and queue there if they are empty or much shorter than this
> CPU's worker queue.]
>
> I also kept open the possibility of introducing multiple worker threads
> per CPU in the future. But having a single-threaded an per-CPU behavior in
> a single API looks quite hard.
>
> we could perhaps do the following, add a single branch to the queue_work()
> fastpath:
>
> if (!cwq->thread)
> cwq = wq->cpu_wq;
Yeah, that was what I was thinking of... if you want to avoid the
branch in the fastpath, we could do something like
struct workqueue_s {
cpu_workqueue_t *cpu_wq_ptr[NR_CPUS]
cpu_workqueue_t cpu_wq[NR_CPUS];
};
and initialize the cpu_wq_ptr entries to
cpu_wq_ptr[i] = cpu_wq + i
in the multi-threaded case and to
cpu_wq_ptr[i] = cpu_wq
in the single thread case. The lookup in queue_work() becomes
cpu_workqueue_t *cwq = wq->cpu_wq[cpu];
which costs an extra pointer deref. Not sure if it's better though.
Alternatively, the !cwq->thread test you're suggesting could be marked
unlikely() so that the multi threaded case which will be used for
performance critical workqueues will be favoured by the compiler.
> and the single-thread queue variant would thus fall back to using a single
> queue only. Plus some sort of new variant could be used:
>
> struct workqueue_struct *create_single_workqueue(char *name);
>
> [or a 'flags' argument to create_workqueue();]
>
> The runtime thing looks slightly ugly, but i think it's acceptable. Has
> anyone any better idea?
>
> > Another minor comment: why do you kmalloc() the workqueue_t? Wouldn't
> > it be more flexible to allow the user to provide a pointer to a
> > pre-allocated workqueue_t structure, e.g.:
> >
> > static workqueue_t aio_wq;
> >
> > [...]
> >
> > create_workqueue(&aio_wq, "aio");
>
> yes, but every creation/destruction use of workqueues is in some sort of
> init/shutdown very-slow-path, so efficiency is not a factor. But clarity
> of the code is a factor, and in the dynamic allocation case it's much
> cleaner to have this:
>
> wq = create_workqueue("worker");
> if (!wq)
> goto error;
>
> than:
>
> wq = kmalloc(sizeof(*wq), GFP_KERNEL);
> if (!wq)
> goto error;
> if (create_workqueue(wq, "worker")) {
> kfree(w);
> goto error;
> }
Sure code clarity is the biggest factor here, but I was thinking we
could cut down the number of dynamic allocations. My guess is that in
the common case, the workqueue_t is either statically allocated (as in
the case of keventd) or embedded in another struct and allocated as
part of that struct:
struct hpsb_host {
char *name;
/* whatnot */
workqueue_t wq;
};
...
host = kmalloc(sizeof *host, GFP_KERNEL);
if (!host)
goto error:
host->name = "ohci1394";
workqueue_init(&host->wq, "ieee1394");
best regards,
Kristian
[email protected] said:
> Here are corresponding changes to UML code
Applied, thanks.
Jeff