[Patch] sysinfo compatibility

[Patch] sysinfo compatibility

[Christoph Rohland]

Hi Eric and Alan,

sysinfo does use a new mem_unit field if ram+swap > MAX_ULONG. That
breaks 2.2 compatibility for a lot machines.

I think it is more resonable to use the mem_unit field only if one of
ram or swap is bigger than MAX_ULONG. (And 2.2 was only broken in that
case)

The appended patch implements that (and makes the logic a little bit
easier)

Greetings
		Christoph

diff -uNr 2.4.9/kernel/info.c 2.4.9-sysinfo/kernel/info.c
--- 2.4.9/kernel/info.c	Sat Apr 21 01:15:40 2001
+++ 2.4.9-sysinfo/kernel/info.c	Wed Jul  4 16:56:23 2001
@@ -16,6 +16,7 @@
 asmlinkage long sys_sysinfo(struct sysinfo *info)
 {
 	struct sysinfo val;
+	unsigned int mem_unit;
 
 	memset((char *)&val, 0, sizeof(struct sysinfo));
 
@@ -32,47 +33,36 @@
 	si_meminfo(&val);
 	si_swapinfo(&val);
 
-	{
-		unsigned long mem_total, sav_total;
-		unsigned int mem_unit, bitcount;
-
-		/* If the sum of all the available memory (i.e. ram + swap)
-		 * is less than can be stored in a 32 bit unsigned long then
-		 * we can be binary compatible with 2.2.x kernels.  If not,
-		 * well, in that case 2.2.x was broken anyways...
-		 *
-		 *  -Erik Andersen <andersee@debian.org> */
-
-		mem_total = val.totalram + val.totalswap;
-		if (mem_total < val.totalram || mem_total < val.totalswap)
-			goto out;
-		bitcount = 0;
-		mem_unit = val.mem_unit;
-		while (mem_unit > 1) {
-			bitcount++;
-			mem_unit >>= 1;
-			sav_total = mem_total;
-			mem_total <<= 1;
-			if (mem_total < sav_total)
-				goto out;
-		}
-
-		/* If mem_total did not overflow, multiply all memory values by
-		 * val.mem_unit and set it to 1.  This leaves things compatible
-		 * with 2.2.x, and also retains compatibility with earlier 2.4.x
-		 * kernels...  */
+	/*
+	 * If the the available memory or swap is less than can be
+	 * stored in a 32 bit unsigned long then we can be binary
+	 * compatible with 2.2.x kernels.  If not, well, in that case
+	 * 2.2.x was broken anyways...
+	 *
+	 *  -Erik Andersen <andersee@debian.org> 
+	 */
+
+	mem_unit = val.mem_unit;
+	if (val.totalram  * mem_unit > val.totalram &&
+	    val.totalswap * mem_unit > val.totalswap) {
+
+		/*
+		 * If mem_total did not overflow, multiply all memory
+		 * values by val.mem_unit and set it to 1.  This
+		 * leaves things compatible with 2.2.x, and also
+		 * retains compatibility with earlier 2.4.x kernels...
+		 */
 
 		val.mem_unit = 1;
-		val.totalram <<= bitcount;
-		val.freeram <<= bitcount;
-		val.sharedram <<= bitcount;
-		val.bufferram <<= bitcount;
-		val.totalswap <<= bitcount;
-		val.freeswap <<= bitcount;
-		val.totalhigh <<= bitcount;
-		val.freehigh <<= bitcount;
+		val.totalram  *= mem_unit;
+		val.freeram   *= mem_unit;
+		val.sharedram *= mem_unit;
+		val.bufferram *= mem_unit;
+		val.totalswap *= mem_unit;
+		val.freeswap  *= mem_unit;
+		val.totalhigh *= mem_unit;
+		val.freehigh  *= mem_unit;
 	}
-out:
 	if (copy_to_user(info, &val, sizeof(struct sysinfo)))
 		return -EFAULT;
 	return 0;

Re: [Patch] sysinfo compatibility

[Hugh Dickins]

On 21 Aug 2001, Christoph Rohland wrote:
> 
> sysinfo does use a new mem_unit field if ram+swap > MAX_ULONG. That
> breaks 2.2 compatibility for a lot machines.
> 
> I think it is more resonable to use the mem_unit field only if one of
> ram or swap is bigger than MAX_ULONG. (And 2.2 was only broken in that
> case)

It's arguable.  When I went there a few months back, I was a little
surprised by the way it adds ram+swap (it mistakenly added in more
before) to make that decision; but concluded it was helping callers
who might well go on to add ram+swap, and felt no overriding reason
to change that.  But you can certainly argue that's inappropriate
for the kernel to do, that it should only guard the validity of
the actual numbers it returns to the caller.  No strong feelings.

> The appended patch implements that (and makes the logic a little bit
> easier)

Alan, please don't apply.  The patch made the logic a lot easier,
but sadly wrong: try what happens to totalswap 0x00120000 with
mem_unit 0x1000 - wrong decision since 0x20000000 > 0x00120000.

Hugh

Re: [Patch] sysinfo compatibility

[Alan Cox]

> before) to make that decision; but concluded it was helping callers
> who might well go on to add ram+swap, and felt no overriding reason
> to change that.  But you can certainly argue that's inappropriate

There are callers who did add ram + swap

Re: [Patch] sysinfo compatibility

[Christoph Rohland]

Hi Hugh,

On Tue, 21 Aug 2001, Hugh Dickins wrote:
> On 21 Aug 2001, Christoph Rohland wrote:
>> 
>> sysinfo does use a new mem_unit field if ram+swap > MAX_ULONG. That
>> breaks 2.2 compatibility for a lot machines.
>> 
>> I think it is more resonable to use the mem_unit field only if one
>> of ram or swap is bigger than MAX_ULONG. (And 2.2 was only broken
>> in that case)
> 
> It's arguable.  When I went there a few months back, I was a little
> surprised by the way it adds ram+swap (it mistakenly added in more
> before) to make that decision; but concluded it was helping callers
> who might well go on to add ram+swap, and felt no overriding reason
> to change that.  But you can certainly argue that's inappropriate
> for the kernel to do, that it should only guard the validity of
> the actual numbers it returns to the caller.  No strong feelings.

I think it's not the kernels task to fix simple user space errors by
breaking compatibility.

And I have somewhat harder feelings since we get a lot of bug reports
that our installer only detects 0M RAM when running on a 2.4 machine
while it works with the 2.2 kernel. We are talking about an ABI which
is directly imported into user space programs.

>> The appended patch implements that (and makes the logic a little
>> bit easier)
> 
> Alan, please don't apply.  The patch made the logic a lot easier,
> but sadly wrong: try what happens to totalswap 0x00120000 with
> mem_unit 0x1000 - wrong decision since 0x20000000 > 0x00120000.

Uh, oh. Try this one instead:

Greetings
		Christoph

--- 8-ac8/kernel/info.c	Tue Aug 21 09:54:02 2001
+++ u8-ac8/kernel/info.c	Tue Aug 21 13:51:42 2001
@@ -16,6 +16,7 @@
 asmlinkage long sys_sysinfo(struct sysinfo *info)
 {
 	struct sysinfo val;
+	unsigned int mem_unit;
 
 	memset((char *)&val, 0, sizeof(struct sysinfo));
 
@@ -32,47 +33,36 @@
 	si_meminfo(&val);
 	si_swapinfo(&val);
 
-	{
-		unsigned long mem_total, sav_total;
-		unsigned int mem_unit, bitcount;
-
-		/* If the sum of all the available memory (i.e. ram + swap)
-		 * is less than can be stored in a 32 bit unsigned long then
-		 * we can be binary compatible with 2.2.x kernels.  If not,
-		 * well, in that case 2.2.x was broken anyways...
-		 *
-		 *  -Erik Andersen <andersee@debian.org> */
-
-		mem_total = val.totalram + val.totalswap;
-		if (mem_total < val.totalram || mem_total < val.totalswap)
-			goto out;
-		bitcount = 0;
-		mem_unit = val.mem_unit;
-		while (mem_unit > 1) {
-			bitcount++;
-			mem_unit >>= 1;
-			sav_total = mem_total;
-			mem_total <<= 1;
-			if (mem_total < sav_total)
-				goto out;
-		}
-
-		/* If mem_total did not overflow, multiply all memory values by
-		 * val.mem_unit and set it to 1.  This leaves things compatible
-		 * with 2.2.x, and also retains compatibility with earlier 2.4.x
-		 * kernels...  */
+	/*
+	 * If the the available memory or swap is less than can be
+	 * stored in a 32 bit unsigned long then we can be binary
+	 * compatible with 2.2.x kernels.  If not, well, in that case
+	 * 2.2.x was broken anyways...
+	 *
+	 *  -Erik Andersen <andersee@debian.org> 
+	 */
+
+	mem_unit = val.mem_unit;
+	if (val.totalram  <= ULONG_MAX / mem_unit &&
+	    val.totalswap <= ULONG_MAX / mem_unit) {
+
+		/*
+		 * If mem_total did not overflow, multiply all memory
+		 * values by val.mem_unit and set it to 1.  This
+		 * leaves things compatible with 2.2.x, and also
+		 * retains compatibility with earlier 2.4.x kernels...
+		 */
 
 		val.mem_unit = 1;
-		val.totalram <<= bitcount;
-		val.freeram <<= bitcount;
-		val.sharedram <<= bitcount;
-		val.bufferram <<= bitcount;
-		val.totalswap <<= bitcount;
-		val.freeswap <<= bitcount;
-		val.totalhigh <<= bitcount;
-		val.freehigh <<= bitcount;
+		val.totalram  *= mem_unit;
+		val.freeram   *= mem_unit;
+		val.sharedram *= mem_unit;
+		val.bufferram *= mem_unit;
+		val.totalswap *= mem_unit;
+		val.freeswap  *= mem_unit;
+		val.totalhigh *= mem_unit;
+		val.freehigh  *= mem_unit;
 	}
-out:
 	if (copy_to_user(info, &val, sizeof(struct sysinfo)))
 		return -EFAULT;
 	return 0;

Re: [Patch] sysinfo compatibility

[Christoph Rohland]

Hi Alan,

On Tue, 21 Aug 2001, Alan Cox wrote:
>> before) to make that decision; but concluded it was helping callers
>> who might well go on to add ram+swap, and felt no overriding reason
>> to change that.  But you can certainly argue that's inappropriate
> 
> There are callers who did add ram + swap

And that's a reason to break compatibility?

Greetings
                Christoph

Re: [Patch] sysinfo compatibility

[Erik Andersen]

On Tue Aug 21, 2001 at 07:30:04PM +0200, Christoph Rohland wrote:
> And I have somewhat harder feelings since we get a lot of bug reports
> that our installer only detects 0M RAM when running on a 2.4 machine
> while it works with the 2.2 kernel. We are talking about an ABI which
> is directly imported into user space programs.

Its your lucky day.  Put something like this in your installer,
and your problems will go away:

/* Include our own copy of struct sysinfo to avoid binary compatibility
 * problems with Linux 2.4, which changed things.  Grumble, grumble. */
struct sysinfo {
    long uptime;            /* Seconds since boot */
    unsigned long loads[3];     /* 1, 5, and 15 minute load averages */
    unsigned long totalram;     /* Total usable main memory size */
    unsigned long freeram;      /* Available memory size */
    unsigned long sharedram;    /* Amount of shared memory */
    unsigned long bufferram;    /* Memory used by buffers */
    unsigned long totalswap;    /* Total swap space size */
    unsigned long freeswap;     /* swap space still available */
    unsigned short procs;       /* Number of current processes */
    unsigned short pad;         /* Padding needed for m68k */
    unsigned long totalhigh;    /* Total high memory size */
    unsigned long freehigh;     /* Available high memory size */
    unsigned int mem_unit;      /* Memory unit size in bytes */
    char _f[20-2*sizeof(long)-sizeof(int)]; /* Padding: libc5 uses this.. */
};
extern int sysinfo (struct sysinfo* info);


/* How much memory does this machine have?
   Units are kBytes to avoid overflow on 4GB machines */
static int check_free_memory()
{
    struct sysinfo info;
    unsigned int result, u, s=10;

    if (sysinfo(&info) != 0) {
        fprintf(stderr,"Error checking free memory");
        return -1;
    }

    /* Kernels 2.0.x and 2.2.x return info.mem_unit==0 with values in bytes.
     * Kernels 2.4.0 return info.mem_unit in bytes. */
    u = info.mem_unit;
    if (u==0) u=1;
    while ( (u&1) == 0 && s > 0 ) { u>>=1; s--; }
    result = (info.totalram>>s) + (info.totalswap>>s);
    result = result*u;
    if (result < 0) result = INT_MAX;
    return result;
}

 -Erik

--
Erik B. Andersen   email:  andersee@debian.org, andersen@lineo.com
--This message was written using 73% post-consumer electrons--

Re: [Patch] sysinfo compatibility

[Alan Cox]

> > There are callers who did add ram + swap
> And that's a reason to break compatibility?
 
We had to break compatibility anyway for 2.4

Re: [Patch] sysinfo compatibility

[Hugh Dickins]

On 21 Aug 2001, Christoph Rohland wrote:
> 
> I think it's not the kernels task to fix simple user space errors by
> breaking compatibility.

This code, either way, is all about trying to keep compatibility with
2.2.  You know some instances which are broken by the current code,
Alan knows some instances which would be broken by your code.

> And I have somewhat harder feelings since we get a lot of bug reports
> that our installer only detects 0M RAM when running on a 2.4 machine
> while it works with the 2.2 kernel. We are talking about an ABI which
> is directly imported into user space programs.

So fix your installer to work with either, isn't that what really
needs to be done?  However you play with 2.4's sysinfo, your current
2.2 installer will give wrong results on some RAM sizes, won't it?

> Uh, oh. Try this one instead:

I think this one is correct (and much clearer than what's currently
there).  But whether it should be applied is for Alan to decide...

Hugh

Re: [Patch] sysinfo compatibility

[Christoph Rohland]

Hi Alan,

On Tue, 21 Aug 2001, Alan Cox wrote:
>> > There are callers who did add ram + swap
>> And that's a reason to break compatibility?
>  
> We had to break compatibility anyway for 2.4

Only for machines with more than 4G swap or ram, not swap +
ram. My patch addresses only this case where we did not fix anything
(for the kernel space) but broke something. Please keep in mind that
2-4 GB machines where quite common also for 2.2.

Greetings
		Christoph

Re: [Patch] sysinfo compatibility

[Christoph Rohland]

Hi Erik,

On Tue, 21 Aug 2001, Erik Andersen wrote:
> Its your lucky day.  Put something like this in your installer,
> and your problems will go away:

We fixed the installer for our upcoming release. But we cannot change
the CDs which were assembled and delivered some time ago. And yes, I
know how to use LD_PRELOAD...

BTW I appreciate the basics of the change for 2.4, but I don't agree
that we should break cases which worked before. (And the comment in
the sources is plain wrong that 2.2 failed in these cases)

Greetings
		Christoph

Re: [Patch] sysinfo compatibility

[Erik Andersen]

On Wed Aug 22, 2001 at 08:44:39AM +0200, Christoph Rohland wrote:
> 
> BTW I appreciate the basics of the change for 2.4, but I don't agree
> that we should break cases which worked before. (And the comment in
> the sources is plain wrong that 2.2 failed in these cases)

But 2.2 _did_ fail.  If you take a linux 2.2.x system, add 4 Gigs
of swap, and then use sysinfo(), the sizes you get back are junk...  

 -Erik

--
Erik B. Andersen   email:  andersee@debian.org, andersen@lineo.com
--This message was written using 73% post-consumer electrons--

Re: [Patch] sysinfo compatibility

[Christoph Rohland]

Hi Erik,

On Wed, 22 Aug 2001, Erik Andersen wrote:
> On Wed Aug 22, 2001 at 08:44:39AM +0200, Christoph Rohland wrote:
>> 
>> BTW I appreciate the basics of the change for 2.4, but I don't
>> agree that we should break cases which worked before. (And the
>> comment in the sources is plain wrong that 2.2 failed in these
>> cases)
> 
> But 2.2 _did_ fail.  If you take a linux 2.2.x system, add 4 Gigs of
> swap, and then use sysinfo(), the sizes you get back are junk...

But if you add 3.9GB it is ok. Also with 3.9GB RAM. And that's a quite
common machine in our environment.

If one of these overflows I agree that the 2.4 scheme is better. But
we should keep compatibility as long as we have no single field which
overflows.

And that's what my patch implements.

Greetings
		Christoph