Re: seperator error in __mask_snprintf_len

[Matthew Dobson]

FWIW, I think a 32-bit chunk-size is preferable.  I personally don't 
think "dead,beef" is easier to read than "deadbeef", nor "0073,0f0f" 
compared to "00730f0f".  On the small CPU count, ie: <=32, either 
version is pretty readable, but on larger CPU count boxen you're going 
to overflow your brain counting groups of 4 versus groups of 8.  I can't 
believe I just called a 32 CPU box "small".  My world perspective is a 
bit skewed... ;)

Also, on the input side, a lot of apps will output a 32-bit CPU mask. 
With commas separating every 32 bits, we can feed an "uncommafied" mask 
to the kernel and it won't barf.  If we go with 16-bit chunks, we'll 
have to "commafy" these 32-bit bitmasks to feed them to the kernel.

As a NUMA guy who deals with largish CPU count machines daily, that's my 
2 cents...

-Matt


Paul Jackson wrote:
> > This patch captures what I am looking for in bitmap display and input.
>
>
> Interesting.  I appear to have provoked Joe into a burst of coding.
> Now, if I had any smarts, I would stand aside and let Joe own this,
> just as Bill Irwin did when I posted my initial lib/mask.c patch a
> couple months ago.
>
>
> Andrew:
>
>     If you find Joe's coding more to your liking than my "Gad" style,
>     I will bless this, and after tossing in a few parting shots, will
>     stand aside.  It meets my essential needs, which were:
>      - chunked output (a comma every 16 or 32 bits),
>      - symmetric input and output formats, and
>      - display and parsing code generic to diverse bitmap sizes.
>
>     My actual recommendation, if however you are still undecided, is:
>      - my patch of last night (with the M32X() 64 bit big endian fix),
>      - Joe's recommended format, zero-filling to chunksize each word,
>      - Joe's renaming/refactoring from lib/mask.c to lib/bitmap.h, and
>      - a chunksize of 16 rather than 32 (Joe likes 16, I don't care).
>
> The essential differences between Joe's and my proposals that I see are:
>     - Joe's has more code, especially in the parsing routine,
>     - Joe's bitmap size resolution is bits, not words, and
>     - I use an implied alloca of 4 x sizeof(mask) bytes.
>
>
> Comments on Joe's patch:
>
>
> > ChangeLog:
>
>
>   Good job of summarizing for the Changelog the changes.
>
>
> > o move into the bitmap.h family, rename and refactor interface to match
>
>
>   I think I like this - good.
>
>
> > o bitmap size resolution changed from byte to bit
>
>
>   Why?  This adds a fair bit of complexity to the code, I suspect.
>   I am not aware of a need for this, but if there is one, ok.
>
>
> > o chunking (digits between commas) changed from 8 to 4 digits
>
>
>   Ok - either 8 or 4 works for me.  This detail should be decided
>   by those working on 16 to 32 cpu systems, who will notice this
>   choice the most.  Those of us on larger or smaller systems are
>   going to see, or not see, separators in either case.
>
>
> > o display no longer affected by sizeof(unsigned long).
>
>
>   A bit of a misstatement.  The display was only affected by the
>   chunksize, one of 32 (sizeof(u32)*8) or 16 (CHUNKSZ).
>
>
> > o no alloca usage
>
>
>   True.  Though on the other hand, you need to roll your own parsing
>   code of comma-separated chunks, instead of getting by with using
>   strsep().  So you trade alloca usage for code complexity.  Either
>   way works - coders choice.  I agree that we disagree on this tradeoff.
>
>
> > o works correctly independent of the size of an unsigned long.
>
>
>   And my version doesn't?
>
>
> > o works on big and little endian machine.
>
>
>   With my M32X() eor-1 fix, so did mine.
>
>
> > + * lib/bitmask.c - bitmask manipulation routines too big to go into bitmask.h
>
>
>   Typo?  Did you mean bitmap.c and bitmap.h, not bitmask?
>
>
> > +int bitmap_parse(const char __user *ubuf, unsigned int ubuflen,
> > +        unsigned long *maskp, unsigned int nmaskbits)
>
>
>   This routine has quite a bit more code detail than my corresponding
>   parsing routine.  I hope that:
>     (1) providing bit-level resolution, and
>     (2) removing the implied alloca
>   justifies this increase in code detail.
>
>
> > +			if (isspace(c))
> > +				continue;
>
>
>   So a space embedded in a hex number is skipped?  That is, your code
>   parses "dead,beef" and "de a d, bee  f" the same?  This seems strange.
>   Perhaps you would prefer to suppress only leading spaces in each chunk:
>
> 			if (!n && isspace(c))
> 				continue;
>
>
> > +			for (j = 0; j < 32; j++) {
>
>
>   What's this "32", an unrepentant CHUNKSZ?


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/