Re: libcruft/sysconf_cpus.c patch

On Wed, November 24, 2010 02:58, Bela Lubkin wrote:
> Indan Zupancic wrote:
>
>> (Bela forgot to CC the list.)
>
> Actually I completely missed my aim, intended to send only to the list
> and you'd get your list copy.

That's what happens by default if you do reply-all, at least for me.

>> > It'll also be wrong if you were exec'd by someone who used
>> > sched_setaffinity() to restrict your horizons.
>>
>> Not really. The manpage says:
>>
>>         - _SC_NPROCESSORS_CONF
>>               The number of processors configured.
>>
>>         - _SC_NPROCESSORS_ONLN
>>               The number of processors currently online (available).
>>
>> I'd argue that for ONLN returning the number of available cpus is the
>> correct thing to do.
>
> Hmmm.  _SC_NPROCESSORS_ONLN is a non-POSIX extension.  glibc doc at
> <http://www.gnu.org/s/libc/manual/html_node/Processor-Resources.html>
> says:

It seems both are non-POSIX:

"These values also exist, but may not be standard."

And the Dragonfly manpage says:

    "_SC_NPROCESSORS_CONF and _SC_NPROCESSORS_ONLN are nonstandard, but imple-
     mented in many systems."

So I'd argue that dietlibc is free to do what it wants here. Everyone using
this interface wants to know how many cpus they can actually use anyway.

This you can find out with with one sched_getaffinity() call. Then there's
the theoretical case of more cpus becoming available after the call, but as
this is very unlikely and so obscure that I don't think dietlibc should
bother getting this corner case right.

> " But it might be possible for the operating system to disable individual
> " processors and so the call
> "
> "        sysconf (_SC_NPROCESSORS_ONLN)
> "
> " returns the number of processors which are currently inline (i.e.,
> " available).
>
> There are at least 3 numbers one *could* want to know: (1) # CPUs in the
> hardware, (2) # CPUs activated by the system (e.g. if the OS has CPU
> licenses, or detected some are broken), and (3) # CPUs *this* process is
> allowed to access.  The doc says (to me) #2.

Knowing 1 gives you nothing useful, neither does 2 if 3 is different.

> Anyway, the number returned should be the same as glibc on the same
> platform.  FWIW, glibc counts "cpu%d" subdirectories in
> /sys/devices/system/cpu for _SC_NPROCESSORS_CONF, and "^cpu%d" lines in
> /proc/stat for _SC_NPROCESSORS_ONLN.  Is /proc/stat format at least as
> stable as (not-very-stable) /proc/cpuinfo across Linux arches &
> releases?  Nothing in dietlibc uses /sys/... yet; I don't think that's a
> good bridge to cross even though current Linux conveniently provides
> /sys/devices/system/cpu/present (#1) and online (#2).  Added in 2008-12
> according to linux/Documentation/ABI/testing/sysfs-devices-system-cpu
> and Documentationlinux//cputopology.txt.

There is no reason to return the same values as glibc, not for this.

If you want to parse /proc/stat, you might want to look at this patch
for uClibc from Bernhard Reutner-Fischer:

http://www.mail-archive.com/[email protected]/msg05261.html

>> Anyway, if you don't want that, and also want to support CONF you can
>> save the original mask, then figure out the number of cpus with
>> sched_setaffinity(), before restoring the original. Though that is
>> more bother than really worth it.
>
> You're assuming the kernel get/set implementation will clear bits above
> the actual # CPUs in the system.  It could just as reasonably let you
> set as many bits as you want, knowing the scheduler will only pay
> attention to the ones that count.

No, I don't assume that. I checked the kernel code. And yes, it does
the right thing (it ANDs the supplied mask with the available cpus).

And unless Linux changes the behaviour and breaks the ABI this will
always work, totally platform independently, unlike parsing /proc
which is both platform dependent and may change in the future.

>> > Anyway, I haven't noticed sched_[gs]etaffinity() support in dietlibc!
>>
>> The kernel supports it, it's a system call, so adding it to dietlibc
>> is trivial.
>
> The stuff in glibc is uhhhgly, feel free...

Probably emulation code or something, if the kernel supports it it's
just a system call.

>> > Writing a fully working /proc version isn't hard... stand by.
>
> I came up with this.  It compiles to 51 bytes more than the old one for
> me (gcc 4.4.5/x86 32-bit), but that's the price for working 100%.  Also,
> it assumes strlen("constant string") is evaluated at compile time -- I'm
> not sure if that's true for all compilers dietlibc might see.  If not,
> NR_CPUS_LEN should be defined in each lobe of that #ifdef.
>
> Retained existing style; changed `i' -> `idx', `n' -> `nb' for better
> greppability; tested with smaller buffers to make sure the algorithm was
> stable.  Changed (buf[m]=='x' && memcmp(buf+m,"xyz",n)) to just memcmp()
> since it's only one per input line and memcmp() returns on 1st mismatch
> anyway.
>
> So, new libcruft/sysconf_cpus.c (and it should definitely be mentioned
> in PORTING):
>
> ========================================================================
> #include <unistd.h>
> #include <string.h>
> #include <fcntl.h>
> #include <errno.h>
> #include <stdlib.h>
> #include "dietfeatures.h"
>
> /*
>  * by Olaf Dreesen
>  * rewritten by Bela Lubkin to handle buffer wrap
>  *
>  * arm			NO SMP ?!? (return 1)
>  * undef SLASH_PROC_OK  NO METHOD! (return 1)
>  *
>  * alpha	->	cpus detected\t\t: <nr>\n
>  * sparc	->	ncpus active\t: <nr>\n
>  *
>  * default	->	processor\t: <cpunr>\n	(one per cpu)
>  */
>
> #if !defined(SLASH_PROC_OK) || defined(__arm__)
> int __sc_nr_cpus() {
>   return 1;	/* kludge kludge ;-) */
> }

Some new ARMs have SMP support.

> #else
> #if defined(__alpha__)
> #define NR_CPUS_STR "cpus detected"
> #define NR_CPUS_OFF 17
> #elif defined(__sparc__)
> #define NR_CPUS_STR "ncpus active"
> #define NR_CPUS_OFF 15
> #else
> #define NR_CPUS_STR "processor"
> #define NR_CPUS_OFF 0 /* count instances of ^NR_CPUS_STR */
> #endif
> #define NR_CPUS_LEN (strlen(NR_CPUS_STR))
> int __sc_nr_cpus(void);
> int __sc_nr_cpus() {
> #if NR_CPUS_OFF == 0
>   int nr=0;
> #endif
>   char buf[1024]; /* holds ~2 cpuinfos */
>   register int nb;
>   int wrap=0;
>   int fd=open("/proc/cpuinfo", O_RDONLY);
>
>   if (fd==-1) return 1; /* fallback if no proc-fs mounted */
>
>   while((nb=read(fd,buf+wrap,sizeof(buf)-wrap))>0 || wrap) {
>     register int idx=0;
>     if (nb<0) nb=0;				/* wrapped buffer after err */
>     nb+=wrap;
>     wrap=0;
>     while (idx<nb) {
>       if (idx+256>=sizeof(buf)) {		/* avoid split-line parsing */
> 	memcpy(buf,buf+idx,(wrap=nb-idx));	/* save tail of full buf */
> 	break;
>       }
>       if (!memcmp(buf+idx,NR_CPUS_STR,NR_CPUS_LEN)) {
> #if NR_CPUS_OFF == 0
> 	++nr;
> #else
> 	close(fd);
> 	return atoi(buf+idx+NR_CPUS_OFF);
> #endif
>       }
>       while(idx<nb && buf[idx++]!='\n');	/* skip rest of line */
>     }
>   }
> #if NR_CPUS_OFF == 0
>   close(fd);
>   return nr;
> #endif
> }
> #endif

Shouldn't you check if nr is greater than 0, to catch untested platforms?

For comparison, the same using sched_getaffinity() would look something
like this:

#include <sched.h>

#define NR 8

int __sc_nr_cpus(void)
{
	int i, c;
	unsigned long v;
	unsigned long m[NR];

	if (sched_getaffinity(0, sizeof(m), &m))
		return 1;
	for (c = 0, i = 0; i < NR; i++){
		/* Peter Wegner/Derrick Lehmer/Brian Kernighan's method */
		for (v = m[i]; v; c++){
		  v &= v - 1; // clear the least significant bit set
		}
	}
	return c;
}

Greetings,

Indan