From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alan Jenkins Date: Mon, 01 Jun 2009 13:46:17 +0000 Subject: Re: [GIT] Experimental threaded udev Message-Id: <4A23DBA9.30501@tuffmail.co.uk> List-Id: References: <4A1EA138.10400@tuffmail.co.uk> In-Reply-To: <4A1EA138.10400@tuffmail.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-hotplug@vger.kernel.org Kay Sievers wrote: > On Mon, Jun 1, 2009 at 11:29, Alan Jenkins wrote: > >>> The thread- and worker-versions do not create as many COW page-faults in >>> the daemon after every cloned event-process, and therefore need much >>> less CPU. >>> >>> At least on the dual-core laptop here, the pool of workers seems to be >>> faster than the threads. >>> >>> >> Great, you beat me to it. It makes sense that this would be _bit_ faster >> than threads. As I say I tried to avoid the page faults caused (somehow) by >> forking the extra thread-stack mappings, but it didn't really work out. I'm >> surprised by quite how much faster it is though! >> > > Yeah, the 1.2 sec system time looked a bit too scary. > > >> I have some thoughts which may help in bringing the code up from "rough >> hack" quality :-). >> > > Cool. :) > > >> Aren't signal queues unreliable? If you exceed the maximum number of queued >> signals, sigqueue will fail with EAGAIN, and I don't think there's a >> blocking version :-). >> > > Right, it's a rlimit, and I think I checked and remember 40.000+ > signals here. The workers could detect, and re-send a non-queued > signal, if needed. > Non-queued signal transmission isn't blocking either. The signal just gets dropped if there's already one in-flight. > >> I think a pipe would be better >> > > Maybe. I was lazy and tried to avoid file descriptors, in case we get > a really large number of workers to maintain. :) > > >> or maybe you can do something with netlink. >> > > Right, but then, I guess, we would need to do MSG_PEEK, or something, > to find out if the main daemon received a kernel message or a worker > message, before trying to do a receive_device(). > Ok. I meant we could send all the completions down the same pipe, which wouldn't require lots of FDs. Before this patch, we were happily sharing kernel_monitor->sock between all the child processes. >> + /* wait for more device messages from udevd */ >> + do >> + dev = udev_monitor_receive_device(monitor); >> + while (dev = NULL); >> >> I think this loop should finish when the signal handler sets worker_exit? >> But maybe you didn't actually install a handler for SIGTERM and it's still >> being reset to the default action: >> > > Yeah, it's not handled with worker_exit. Now, it's not reliable to > kill event processes from something else than the main daemon. The > worker_exit might be nice for valgrind tests though. > > >> I'm not sure you're handling worker processes crashing, it looks like you >> might leave the event in limbo if you get SIGCHLD for a worker in state >> WORKER_RUNNING. I'm sure you'll test that though. >> > > Maybe we can set the event to QUEUED, when a worker dies with an event attached. > Retry the event, you mean? How many times? :-). The code looked like you freed the worker, but left the event RUNNING, and it would never be released. I would delete the event instead, just like the old system. I haven't read V2 yet though, maybe you fixed it. > >> Talking of unexpected crashes. I anticipated using a connection-oriented >> socketpair for passing events. Using netlink for this means the worker >> threads don't get a nice notification if the daemon dies without killing >> them. >> > > Right, thats not too nice. I guess we have pretty much lost if the > main daemon dies unexpectedly. We need to find out, if we want netlink > and signals, or socketpair()s here, I guess. > > >> Unless this is covered by udevd being the session leader, or >> something like that? I'll test this empirically - maybe it's not important, >> but I think it's good to know what will happen. >> > > If the main daemon exits normally, it sends TERM to the entire process group > > >> BTW I had a go at this too, but a couple of my workers fail in about 1 run >> out of 20, apparently because calloc() returns NULL without setting errno. >> I'm sure it's my fault but I'll try to track it down. Obviously I'll let >> you know if your patch could be affected by the same problem. >> > > Oh, strange. Would be good to know what causes this. > Don't worry about it, it was just my buggy code :-). I used event->udev after udev_event_unref(event). Thanks Alan