From: Blaisorblade <bla...@ya...> - 2004-11-05 19:36:43
|
On Friday 05 November 2004 06:48, Jeff Dike wrote: > bla...@ya... said: > > I had a doubt on this, but I was not getting much feedback from you... > Yeah, sorry. Jeff, please read this one - I have a number of important things in it. Also, I'd like comments from everyone else interested. I can understand you, so I'll try to reduce my mails to you to increase the signal/noise ratio. I'll still send what needed to the list, but CC you just when requesting your attention (not for trivial fixlet, yes for things as this one, which smelt like being an hack done on purpose). > > Also, if you reject this, I'd require a comment-only patch for it: "as > > soon as I remember why" makes me think back to my yesterday's class, > > when the teacher said "put comments in your code or you'll soon > > forget what it does!" 8-O (yes, 1st year University student :-( ). > The thing is, you often don't realize what's going to be mysterious until > it actually is, and then it's too late for the comment :-) > In this case, it wants to be bounced out You mean it failing with EINTR, right? > of sigprocmask when a SIGWINCH > arrives. Also, why shouldn't sigprocmask be restartable with the -ERESTART* mechanism? Wouldn't your kludge break? Also, a nicer way to code this could be to have an explicit sighandler setting a flag (to get the syscall interrupted if the signal arrives before being blocked) and to call sigpending() (to test if the signal arrived just after setting it). After the syscall, that could become SIG_IGN. Also, (optional answer), why is this needed? A comment about such issues would be better than an answer email. > In order to do so, it must have a handler registered, even if > it does nothing. Ok. However, I have a general question about all this whole code: why do you use pipes as synchronization primitives? Did you avoid semaphores for portability issues, or for persistency ones? Both can be solved (with the os_ layer and the IPC_PRIVATE key). This would especially help during context switching, I think. I have just a rough idea of what switch_pipe is for, but calling the network layer (what you call os_pipe() is actually socketpair(), which is very confusing) to rely on the semaphores / wait queues it uses seems suboptimal and ugly. What are your ideas about this? -- Paolo Giarrusso, aka Blaisorblade Linux registered user n. 292729 |
From: Jeff D. <jd...@ad...> - 2004-11-06 03:01:14
|
On Fri, Nov 05, 2004 at 08:36:55PM +0100, Blaisorblade wrote: > Also, why shouldn't sigprocmask be restartable with the -ERESTART* mechanism? Err, that was pause, not sigprocmask. sigprocmask just switches the signal mask. pause is what sits there and waits for something to happen. > Wouldn't your kludge break? What kludge? > Also, a nicer way to code this could be to have an explicit sighandler setting > a flag (to get the syscall interrupted if the signal arrives before being > blocked) and to call sigpending() (to test if the signal arrived just after > setting it). After the syscall, that could become SIG_IGN. You're seeing races where there aren't any. SIGWINCH is only possible when it gets a controlling tty, which happens after the sigprocmask. > Also, (optional answer), why is this needed? A comment about such issues would > be better than an answer email. The point of this is to handle SIGWINCH on consoles which have host ttys and relay them inside UML to whatever might be running on the console and cares about the window size. So, we have a separate thread for each host tty attached to a UML device (side-issue - I'm annoyed that one thread can't have multiple controlling ttys for purposed of handling SIGWINCH, but I imagine there are other reasons that doesn't make any sense). SIGWINCH can't be received synchronously, so you have to set up to receive it as a signal. That being the case, if you are going to wait for it, it is convenient to sit in a pause() and wait for the signal to bounce you out of it. So, you need a signal handler for the reason I mentioned before, but it's not needed to do anything. > Ok. However, I have a general question about all this whole code: why do you > use pipes as synchronization primitives? Did you avoid semaphores for > portability issues, or for persistency ones? Both can be solved (with the os_ > layer and the IPC_PRIVATE key). OK, the synchronization here is due to that fact that the argument recieved by the thread is on the stack of the parent. The parent can't return from its current procedure until it knows that the child has copied the data onto its own stack. Hence the writing of the byte after it has been copied. A semaphore would work, and in fact that's exactly what I'm imitating here. I've avoided SysV semaphores because of a long-standing dislike of them, stemming mostly from their persistence, and the fact that they need to be cleaned up manually if the process that created them didn't destroy them. I just had a look at the man page, and I see this: The name choice IPC_PRIVATE was perhaps unfortunate, IPC_NEW would more clearly show its function. So, IPC_PRIVATE ones would seem to be persistent, too. A more basic issue is the interface. What I have now is the mapping open <-> create read <-> down write <-> up close <-> destroy which is way simpler and cleaner than semget, semop, and ??? (I can't figure out how to destroy one of these things). > This would especially help during context switching, I think. I have just a > rough idea of what switch_pipe is for, but calling the network layer (what > you call os_pipe() is actually socketpair(), which is very confusing) to rely > on the semaphores / wait queues it uses seems suboptimal and ugly. For context switching, normal pipes (or semaphores) would be fine. I use socketpairs because they support SIGIO, and pipes don't. I stuck with socketpairs just because they do everything I need. Jeff |
From: Blaisorblade <bla...@ya...> - 2004-11-09 17:44:02
|
On Saturday 06 November 2004 06:13, Jeff Dike wrote: > On Fri, Nov 05, 2004 at 08:36:55PM +0100, Blaisorblade wrote: > > Also, why shouldn't sigprocmask be restartable with the -ERESTART* > > mechanism? > Err, that was pause, not sigprocmask. sigprocmask just switches the sign= al > mask. Ok, I saw it able to return EINTR, so I thought you wanted it to bounce bac= k=20 there (I wasn't understanding the purpose of the code anyway, now with your= =20 help it's clear). > pause is what sits there and waits for something to happen. > > Wouldn't your kludge break? > What kludge? [...] > You're seeing races where there aren't any. SIGWINCH is only possible wh= en > it gets a controlling tty, which happens after the sigprocmask. When writing this I was still thinking to sigprocmask() getting SIGWINCH, n= ot=20 pause. So obviously I said nonsense. I also understand now what all this is for. When I have time for this, I'll= at=20 least copy and paste your mail into a comment, with any needed adjustment. =46or the semaphore issue, I have some ideas (like using futexes) which nee= d to=20 be developed a bit: 1) I want to create a semaphore API in os_*. 2) It will be able to use socketpairs. 3) It will be able to use futexes, if they are non-persistant and usable=20 without too much issues (the same way we are going to support Async I/O). 4) It will be used first by the code which could really benefit from the=20 performance increase. 5) It won't use persistant objects. Any comment on these issues? Also, apart TT context switching, is there any= =20 other performance-sensitive use of semaphores, which would benefit from usi= ng=20 futexes? About this: > A more basic issue is the interface. > What I have now is the mapping=20 > open <-> create > read <-> down > write <-> up > close <-> destroy > which is way simpler and cleaner than semget, semop, and ??? (I can't > figure out how to destroy one of these things). Yes, semget and friends are uglier. But don't think that the current nested code is simple to read - three=20 semaphores at a time, without a clear name, are not the clearer code on the= =20 world. =2D-=20 Paolo Giarrusso, aka Blaisorblade Linux registered user n. 292729 |
From: Jeff D. <jd...@ad...> - 2004-11-09 18:36:21
|
bla...@ya... said: > I also understand now what all this is for. When I have time for this, > I'll at least copy and paste your mail into a comment, with any > needed adjustment. That would be a good idea. > For the semaphore issue, I have some ideas (like using futexes) which > need to be developed a bit: > 1) I want to create a semaphore API in os_*. > 2) It will be able to use socketpairs. > 3) It will be able to use futexes, if they are > non-persistant and usable without too much issues (the same way we > are going to support Async I/O). > 4) It will be used first by the code > which could really benefit from the performance increase. > 5) It won't > use persistant objects. This all sounds good, although there are simplicity benefits to just using one underlying mechanism, as long as there are no overriding disadvantages to it. > Any comment on these issues? Also, apart TT context switching, is > there any other performance-sensitive use of semaphores, which would > benefit from using futexes? Offhand, I think context switching is the most sensitive one. > Yes, semget and friends are uglier. > But don't think that the current nested code is simple to read - three > semaphores at a time, without a clear name, are not the clearer code > on the world. What nested code are you talking about? Jeff |
From: Blaisorblade <bla...@ya...> - 2004-11-09 19:14:39
|
On Tuesday 09 November 2004 21:48, Jeff Dike wrote: > bla...@ya... said: > > I also understand now what all this is for. When I have time for this, > > I'll at least copy and paste your mail into a comment, with any > > needed adjustment. > That would be a good idea. > > For the semaphore issue, I have some ideas (like using futexes) which > > need to be developed a bit: > > 1) I want to create a semaphore API in os_*. > > 2) It will be able to use socketpairs. > > 3) It will be able to use futexes, if they are > > non-persistant and usable without too much issues (the same way we > > are going to support Async I/O). > > 4) It will be used first by the code > > which could really benefit from the performance increase. > > 5) It won't > > use persistant objects. > This all sounds good, although there are simplicity benefits to just using > one underlying mechanism, as long as there are no overriding disadvantages > to it. Yes, I would like that, too, but futexes are 2.6 only, and probably also NPTL-only (we are going to fix that, at least for SKAS mode), but faster than anything else. Nothing apart this. > > Any comment on these issues? Also, apart TT context switching, is > > there any other performance-sensitive use of semaphores, which would > > benefit from using futexes? > Offhand, I think context switching is the most sensitive one. Ok. But to get TT mode to work against NPTL glibc, which is required for futexes, we need to recode the "thread_private" section in uml.lds.S to work with NPTL glibc. It seems that binutils does not like that (the error is "not enough program header allocated", which refers to the fact that SIZEOF_HEADERS is guessed and then used. Not using SIZEOF_HEADERS could help, if doing this is possible). > > Yes, semget and friends are uglier. > > But don't think that the current nested code is simple to read - three > > semaphores at a time, without a clear name, are not the clearer code > > on the world. > What nested code are you talking about? There are two down() and two up(); additionally, run_helper_thread() manages at least another pipe(). I don't see an easy way to simplifying all this, but it's needed (or at least some comment should be added). Just a cleanup, anyway. -- Paolo Giarrusso, aka Blaisorblade Linux registered user n. 292729 |
From: Chris F. <cfr...@no...> - 2004-11-09 19:41:26
|
Blaisorblade wrote: > Yes, I would like that, too, but futexes are 2.6 only, and probably also > NPTL-only (we are going to fix that, at least for SKAS mode), but faster than > anything else. Nothing apart this. Actually, you can use raw futexes directly without needing any thread library. There is even some helper code available if you search around a bit. Chris |