All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andi Kleen <ak@muc.de>
To: Paul Jackson <pj@sgi.com>
Cc: bcasavan@sgi.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH] get_nodes mask miscalculation
Date: 10 Aug 2004 12:45:47 +0200
Date: Tue, 10 Aug 2004 12:45:47 +0200	[thread overview]
Message-ID: <20040810104547.GA59597@muc.de> (raw)
In-Reply-To: <20040809222531.1d2d8d05.pj@sgi.com>

On Mon, Aug 09, 2004 at 10:25:31PM -0700, Paul Jackson wrote:
> While I'm here, the statement that only the highest zone is policied
> actually applies only to MPOL_BIND, right?  The comment that asserts

That is correct. I can add a comment.

>  1) Change the man page wording, for each of get_mempolicy(2), mbind(2),
>     and set_mempolicy(2), to boldly state:
> 
> 	Beware:
> 		Pass in a value of maxnode that is * one more * than the
> 		number of nodes represented in nodemask.  If for example,
> 		nodemask represents 64 nodes, numbered 0 to 63, pass in a
> 		value of 65 for maxnodes.

Yes, I will clarify the manpages.

I see no problem with hardcoding 8 bits per byte. 

> 
>  2) Review, test, fix, and apply as fixed the following patch.  For extra
>     credit, get rid of the hard coded 8, 32 and 64 values in the compat stuff,
>     visible in the patch below.  I compiled the patch, once, on an ia64.
>     Otherwise totally untested.
> 
>     This patch:
> 	a) Notes the situation in a prominent "==> Beware <==".
> 	b) Consistently decrements maxnode immediately on each system call
> 	   entry (where someone reading the code might best notice).
> 	c) Otherwise treats maxnode consistently within the code.
> 	d) Addresses the MPOL_BIND max policy only comment.
> 	e) Addresses the harcoded numbers 64 and 8 in copy_nodes_to_user().

Yes, the 64 should be addressed agreed. That came from a misguided
attempt by me to not require compat_* functions (by making the 32bit
and 64bit ABI be the same), but that didn't work out.

> 
>     Yes - it's ugly.  The time that will be lost by those who try to use
>     this interface directly will be ugly too.
> 
>  3) Could you propose a strategy for fixing this?  It might take a couple

The only way would be to allocate new system call slots for the 
two calls. But I'm not convinced it is worth it.

Sorry, but I don't want to break binary compatibility. 

If it was really that bad you should have complained earlier (the
code was out long enough for review), now it is too late.

> If
>    you have to tell me that SGI has to put in a workaround for a year, on
>    any machine with _exactly_ 2049 nodes, such as padding the user nodemask
>    up to 2050 nodes, I'm prepared to deal with that <grin>.

There are many more users of these calls than SGI. And most of them are
using libnuma, which you are proposing to break. 


-Andi

WARNING: multiple messages have this Message-ID (diff)
From: Andi Kleen <ak@muc.de>
To: Paul Jackson <pj@sgi.com>
Cc: bcasavan@sgi.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH] get_nodes mask miscalculation
Date: 10 Aug 2004 12:45:47 +0200
Date: Tue, 10 Aug 2004 12:45:47 +0200	[thread overview]
Message-ID: <20040810104547.GA59597@muc.de> (raw)
In-Reply-To: <20040809222531.1d2d8d05.pj@sgi.com>

On Mon, Aug 09, 2004 at 10:25:31PM -0700, Paul Jackson wrote:
> While I'm here, the statement that only the highest zone is policied
> actually applies only to MPOL_BIND, right?  The comment that asserts

That is correct. I can add a comment.

>  1) Change the man page wording, for each of get_mempolicy(2), mbind(2),
>     and set_mempolicy(2), to boldly state:
> 
> 	Beware:
> 		Pass in a value of maxnode that is * one more * than the
> 		number of nodes represented in nodemask.  If for example,
> 		nodemask represents 64 nodes, numbered 0 to 63, pass in a
> 		value of 65 for maxnodes.

Yes, I will clarify the manpages.

I see no problem with hardcoding 8 bits per byte. 

> 
>  2) Review, test, fix, and apply as fixed the following patch.  For extra
>     credit, get rid of the hard coded 8, 32 and 64 values in the compat stuff,
>     visible in the patch below.  I compiled the patch, once, on an ia64.
>     Otherwise totally untested.
> 
>     This patch:
> 	a) Notes the situation in a prominent "==> Beware <==".
> 	b) Consistently decrements maxnode immediately on each system call
> 	   entry (where someone reading the code might best notice).
> 	c) Otherwise treats maxnode consistently within the code.
> 	d) Addresses the MPOL_BIND max policy only comment.
> 	e) Addresses the harcoded numbers 64 and 8 in copy_nodes_to_user().

Yes, the 64 should be addressed agreed. That came from a misguided
attempt by me to not require compat_* functions (by making the 32bit
and 64bit ABI be the same), but that didn't work out.

> 
>     Yes - it's ugly.  The time that will be lost by those who try to use
>     this interface directly will be ugly too.
> 
>  3) Could you propose a strategy for fixing this?  It might take a couple

The only way would be to allocate new system call slots for the 
two calls. But I'm not convinced it is worth it.

Sorry, but I don't want to break binary compatibility. 

If it was really that bad you should have complained earlier (the
code was out long enough for review), now it is too late.

> If
>    you have to tell me that SGI has to put in a workaround for a year, on
>    any machine with _exactly_ 2049 nodes, such as padding the user nodemask
>    up to 2050 nodes, I'm prepared to deal with that <grin>.

There are many more users of these calls than SGI. And most of them are
using libnuma, which you are proposing to break. 


-Andi
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

  reply	other threads:[~2004-08-10 10:45 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <2rr7U-5xT-11@gated-at.bofh.it>
2004-08-10  1:44 ` [PATCH] get_nodes mask miscalculation Andi Kleen
2004-08-10  1:44   ` Andi Kleen
2004-08-10  5:25   ` Paul Jackson
2004-08-10  5:25     ` Paul Jackson
2004-08-10 10:45     ` Andi Kleen [this message]
2004-08-10 10:45       ` Andi Kleen
2004-08-17  3:43       ` Paul Jackson
2004-08-17  3:43         ` Paul Jackson
2004-08-10 14:54   ` Brent Casavant
2004-08-10 14:54     ` Brent Casavant
2004-08-10 15:34     ` Andi Kleen
2004-08-10 15:34       ` Andi Kleen
2004-08-09 22:39 Brent Casavant
2004-08-09 22:39 ` Brent Casavant

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20040810104547.GA59597@muc.de \
    --to=ak@muc.de \
    --cc=bcasavan@sgi.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=pj@sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.