From: <bla...@ya...> - 2004-09-12 18:44:57
|
Since local_irq_save() and local_irq_disable() should match (apart from saving the flags), [gs]et_signals must match [un]block_signals, i.e. they must act onto SIGPROF, too. At least IMHO. Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- uml-linux-2.6.8.1-paolo/arch/um/kernel/signal_user.c | 20 +++++++++++++++---- 1 files changed, 16 insertions(+), 4 deletions(-) diff -puN arch/um/kernel/signal_user.c~uml-add-SIGPROF-get-set_signals arch/um/kernel/signal_user.c --- uml-linux-2.6.8.1/arch/um/kernel/signal_user.c~uml-add-SIGPROF-get-set_signals 2004-08-29 14:40:54.102986872 +0200 +++ uml-linux-2.6.8.1-paolo/arch/um/kernel/signal_user.c 2004-08-29 14:40:54.104986568 +0200 @@ -83,14 +83,21 @@ void unblock_signals(void) #define SIGIO_BIT 0 #define SIGVTALRM_BIT 1 +#define SIGPROF_BIT 2 -static int enable_mask(sigset_t *mask) +/* + * Inverts the signal mask: + * @mask: a sigset_t* point to the mask of blocked signals. + * Returns a mask (of different type) of *unblocked* signals. + */ +static inline int enable_mask(const sigset_t *mask) { int sigs; sigs = sigismember(mask, SIGIO) ? 0 : 1 << SIGIO_BIT; sigs |= sigismember(mask, SIGVTALRM) ? 0 : 1 << SIGVTALRM_BIT; sigs |= sigismember(mask, SIGALRM) ? 0 : 1 << SIGVTALRM_BIT; + sigs |= sigismember(mask, SIGPROF) ? 0 : 1 << SIGPROF_BIT; return(sigs); } @@ -103,21 +110,26 @@ int get_signals(void) return(enable_mask(&mask)); } +/*Returns the old mask of the old active signals (not sigset_t, but + * suitable for set_signals.*/ int set_signals(int enable) { sigset_t mask; + sigset_t old_mask; int ret; sigemptyset(&mask); - if(enable & (1 << SIGIO_BIT)) + if(enable & (1 << SIGIO_BIT)) sigaddset(&mask, SIGIO); if(enable & (1 << SIGVTALRM_BIT)){ sigaddset(&mask, SIGVTALRM); sigaddset(&mask, SIGALRM); } - if(sigprocmask(SIG_UNBLOCK, &mask, &mask) < 0) + if(enable & (1 << SIGPROF_BIT)) + sigaddset(&mask, SIGPROF); + if(sigprocmask(SIG_UNBLOCK, &mask, &old_mask) < 0) panic("Failed to enable signals"); - ret = enable_mask(&mask); + ret = enable_mask(&old_mask); sigemptyset(&mask); if((enable & (1 << SIGIO_BIT)) == 0) sigaddset(&mask, SIGIO); _ |
From: <bla...@ya...> - 2004-09-13 19:57:00
|
Since local_irq_save() and local_irq_disable() should match (apart from saving the flags), [gs]et_signals must match [un]block_signals, i.e. they must act onto SIGPROF, too. At least IMHO. Signed-off-by: Paolo 'Blaisorblade' Giarrusso <bla...@ya...> --- uml-linux-2.6.8.1-paolo/arch/um/kernel/signal_user.c | 18 ++++++++++++++++-- 1 files changed, 16 insertions(+), 2 deletions(-) diff -puN arch/um/kernel/signal_user.c~uml-add-SIGPROF-get-set_signals arch/um/kernel/signal_user.c --- uml-linux-2.6.8.1/arch/um/kernel/signal_user.c~uml-add-SIGPROF-get-set_signals 2004-09-12 18:04:27.000000000 +0200 +++ uml-linux-2.6.8.1-paolo/arch/um/kernel/signal_user.c 2004-09-13 20:12:58.544305736 +0200 @@ -83,14 +83,21 @@ void unblock_signals(void) #define SIGIO_BIT 0 #define SIGVTALRM_BIT 1 +#define SIGPROF_BIT 2 -static int enable_mask(sigset_t *mask) +/* + * Inverts the signal mask: + * @mask: a sigset_t* point to the mask of blocked signals. + * Returns a mask (of different type) of *unblocked* signals. + */ +static inline int enable_mask(const sigset_t *mask) { int sigs; sigs = sigismember(mask, SIGIO) ? 0 : 1 << SIGIO_BIT; sigs |= sigismember(mask, SIGVTALRM) ? 0 : 1 << SIGVTALRM_BIT; sigs |= sigismember(mask, SIGALRM) ? 0 : 1 << SIGVTALRM_BIT; + sigs |= sigismember(mask, SIGPROF) ? 0 : 1 << SIGPROF_BIT; return(sigs); } @@ -103,18 +110,25 @@ int get_signals(void) return(enable_mask(&mask)); } +/* Returns the old mask of the old active signals (not sigset_t, but + * suitable for set_signals.*/ int set_signals(int enable) { sigset_t mask; int ret; sigemptyset(&mask); - if(enable & (1 << SIGIO_BIT)) + if(enable & (1 << SIGIO_BIT)) sigaddset(&mask, SIGIO); if(enable & (1 << SIGVTALRM_BIT)){ sigaddset(&mask, SIGVTALRM); sigaddset(&mask, SIGALRM); } + if(enable & (1 << SIGPROF_BIT)) + sigaddset(&mask, SIGPROF); + /* This is safe - sigprocmask is a syscall, so it must copy locally the + * value of new_set, do his work and then, at the end, write to + * old_set.*/ if(sigprocmask(SIG_UNBLOCK, &mask, &mask) < 0) panic("Failed to enable signals"); ret = enable_mask(&mask); _ |
From: Jeff D. <jd...@ad...> - 2004-09-13 21:30:53
|
bla...@ya... said: > Since local_irq_save() and local_irq_disable() should match (apart > from saving the flags), [gs]et_signals must match [un]block_signals, > i.e. they must act onto SIGPROF, too. At least IMHO. Applied with some minor rewording of the comment, thanks. Jeff |
From: Jeff D. <jd...@ad...> - 2004-09-13 02:28:54
|
On Sun, Sep 12, 2004 at 08:26:29PM +0200, bla...@ya... wrote: > { > sigset_t mask; > + sigset_t old_mask; > int ret; Weren't you just complaining about stack consumption? Do you know how big a sigset_t is? (gdb) p sizeof(sigset_t) $1 = 128 I went to a reasonable amount of trouble to get that function to only use one sigset_t and now you are proposing to add the old one back. Fix the patch and I'll put it in. Also, if you're going to fix something, send a patch which does nothing but make the fix, and save whatever cleanups you want to make for a differet patch. Jeff |
From: BlaisorBlade <bla...@ya...> - 2004-09-13 19:11:18
|
On Monday 13 September 2004 05:32, Jeff Dike wrote: > On Sun, Sep 12, 2004 at 08:26:29PM +0200, bla...@ya... wrote: > > { > > sigset_t mask; > > + sigset_t old_mask; > > int ret; > Weren't you just complaining about stack consumption? Do you know how > big a sigset_t is? > (gdb) p sizeof(sigset_t) > $1 = 128 Yes, I realize (glibc seems seriously original about this - who ever can use 1024 different signals? The kernel just uses 64 signals - 8 bit). > I went to a reasonable amount of trouble to get that function to only > use one sigset_t and now you are proposing to add the old one back. > Fix the patch and I'll put it in. I rechecked the patch and understood why I put that in - I understand your point, but since that needs at least some, but using such unobvious tricks requires at least some comment - I'm adding the comment to the patch (and sending it inline separately). Obviously feel free to delete/change the comment. > Also, if you're going to fix something, send a patch which does > nothing but make the fix, and save whatever cleanups you want to make > for a differet patch. Ok, I agree at all, and most times I already work like that. Bye -- Paolo Giarrusso, aka Blaisorblade Linux registered user n. 292729 |
From: Jeff D. <jd...@ad...> - 2004-09-13 21:11:59
|
bla...@ya... said: > I rechecked the patch and understood why I put that in - I understand > your point, but since that needs at least some, but using such > unobvious tricks requires at least some comment - I'm adding the > comment to the patch (and sending it inline separately). Yeah, a comment is the right thing to do there. Jeff |
From: Jeff D. <jd...@ad...> - 2004-09-14 18:23:13
|
Actually, I had second thoughts about this patch. I left SIGPROF out of [un]block_signals on purpose. The reason is that you want to be able to profile all of UML, not just the pieces where signals are enabled. This is safe because the profiling code doesn't share any data with the kernel. A comment to this effect may be warranted, but the signal itself should be left alone. Jeff |
From: BlaisorBlade <bla...@ya...> - 2004-09-17 18:55:53
|
On Tuesday 14 September 2004 21:26, Jeff Dike wrote: > Actually, I had second thoughts about this patch. I left SIGPROF out of > [un]block_signals on purpose. The reason is that you want to be able to > profile all of UML, not just the pieces where signals are enabled. This is > safe because the profiling code doesn't share any data with the kernel. I agree. > A comment to this effect may be warranted, Translate "may" to "should". The Uml code already seems very lacking of comments. > but the signal itself should be > left alone. Ah, ok, but I didn't do that because I saw "well, let's add this, too". I took it from change_signals. In this case, you should remove SIGPROF from change_signals, too. If you think you may have a possibility to make them different, please take a look at the definitions of local_irq_save/restore and local_irq_enable/disable, and realize that local_irq_save and local_irq_disable should have no hidden difference, i.e. they must treat SIGPROF in the same way. If (I guess not) you have any doubt, I'd recommend asking this on LKML, if you feel brave :-)! -- Paolo Giarrusso, aka Blaisorblade Linux registered user n. 292729 |
From: Jeff D. <jd...@ad...> - 2004-09-17 23:40:44
|
On Fri, Sep 17, 2004 at 08:52:48PM +0200, BlaisorBlade wrote: > Translate "may" to "should". The Uml code already seems very lacking of > comments. Methinks someone missed the joke :-) > Ah, ok, but I didn't do that because I saw "well, let's add this, too". I took > it from change_signals. > > In this case, you should remove SIGPROF from change_signals, too. Yup, I didn't see that. Jeff |