[uml-devel] [patch 2/2] add-SIGPROF-get-set_signals

[uml-devel] [patch 2/2] add-SIGPROF-get-set_signals

[blaisorblade_spam]

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 <blaisorblade_spam@yahoo.it>
---

 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);
_


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM. 
Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

[uml-devel] Re: [patch 2/2] add-SIGPROF-get-set_signals

[Jeff Dike]

On Sun, Sep 12, 2004 at 08:26:29PM +0200, blaisorblade_spam@yahoo.it 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


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM. 
Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Re: [patch 2/2] add-SIGPROF-get-set_signals

[BlaisorBlade]

On Monday 13 September 2004 05:32, Jeff Dike wrote:
> On Sun, Sep 12, 2004 at 08:26:29PM +0200, blaisorblade_spam@yahoo.it 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


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM. 
Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Re: [patch 2/2] add-SIGPROF-get-set_signals

[Jeff Dike]

blaisorblade_spam@yahoo.it 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



-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM. 
Deadline: Sept. 13. Go here: http://sf.net/ppc_contest.php
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Re: [patch 2/2] add-SIGPROF-get-set_signals

[Jeff Dike]

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



-------------------------------------------------------
This SF.Net email is sponsored by: thawte's Crypto Challenge Vl
Crack the code and win a Sony DCRHC40 MiniDV Digital Handycam
Camcorder. More prizes in the weekly Lunch Hour Challenge.
Sign up NOW http://ad.doubleclick.net/clk;10740251;10262165;m
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Re: [patch 2/2] add-SIGPROF-get-set_signals

[BlaisorBlade]

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


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Re: [patch 2/2] add-SIGPROF-get-set_signals

[Jeff Dike]

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


-------------------------------------------------------
This SF.Net email is sponsored by: YOU BE THE JUDGE. Be one of 170
Project Admins to receive an Apple iPod Mini FREE for your judgement on
who ports your project to Linux PPC the best. Sponsored by IBM.
Deadline: Sept. 24. Go here: http://sf.net/ppc_contest.php
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel