memfill

memfill

[Matthew Wilcox]

[adding linux-arch to see if anyone there wants to do an optimised
version of memfill for their CPU]

On Mon, Feb 06, 2017 at 12:16:44AM +0900, Minchan Kim wrote:
> +static inline void zram_fill_page(char *ptr, unsigned long len,
> +					unsigned long value)
> +{
> +	int i;
> +	unsigned long *page = (unsigned long *)ptr;
> +
> +	WARN_ON_ONCE(!IS_ALIGNED(len, sizeof(unsigned long)));
> +
> +	if (likely(value == 0)) {
> +		memset(ptr, 0, len);
> +	} else {
> +		for (i = 0; i < len / sizeof(*page); i++)
> +			page[i] = value;
> +	}
> +}

I would suggest:

#ifndef __HAVE_ARCH_MEMFILL
/** 
 * memfill - Fill a region of memory with the given value
 * @s: Pointer to the start of the region.
 * @v: The word to fill the region with.
 * @n: The size of the region.
 * 
 * Differs from memset() in that it fills with an unsigned long instead of 
 * a byte.  The pointer and the size must be aligned to unsigned long.
 */
void memfill(unsigned long *s, unsigned long v, size_t n)
{
	WARN_ON_ONCE(!IS_ALIGNED(n, sizeof(v)));

	if (likely(v == 0)) {
		memset(s, 0, n);
	} else {
		while (n) {
			*s++ = v;
			n -= sizeof(v);
		}
	}
}
EXPORT_SYMBOL(memfill);
#endif

(I would also suggest this move to lib/string.c and architectures be
given the opportunity to provide an optimised version of memfill).

memfill

[Matthew Wilcox]

[adding linux-arch to see if anyone there wants to do an optimised
version of memfill for their CPU]

On Mon, Feb 06, 2017 at 12:16:44AM +0900, Minchan Kim wrote:
> +static inline void zram_fill_page(char *ptr, unsigned long len,
> +					unsigned long value)
> +{
> +	int i;
> +	unsigned long *page = (unsigned long *)ptr;
> +
> +	WARN_ON_ONCE(!IS_ALIGNED(len, sizeof(unsigned long)));
> +
> +	if (likely(value == 0)) {
> +		memset(ptr, 0, len);
> +	} else {
> +		for (i = 0; i < len / sizeof(*page); i++)
> +			page[i] = value;
> +	}
> +}

I would suggest:

#ifndef __HAVE_ARCH_MEMFILL
/** 
 * memfill - Fill a region of memory with the given value
 * @s: Pointer to the start of the region.
 * @v: The word to fill the region with.
 * @n: The size of the region.
 * 
 * Differs from memset() in that it fills with an unsigned long instead of 
 * a byte.  The pointer and the size must be aligned to unsigned long.
 */
void memfill(unsigned long *s, unsigned long v, size_t n)
{
	WARN_ON_ONCE(!IS_ALIGNED(n, sizeof(v)));

	if (likely(v == 0)) {
		memset(s, 0, n);
	} else {
		while (n) {
			*s++ = v;
			n -= sizeof(v);
		}
	}
}
EXPORT_SYMBOL(memfill);
#endif

(I would also suggest this move to lib/string.c and architectures be
given the opportunity to provide an optimised version of memfill).

Re: memfill

[zhouxianrong]

On 2017/2/6 22:49, Matthew Wilcox wrote:
>
> [adding linux-arch to see if anyone there wants to do an optimised
> version of memfill for their CPU]
>
> On Mon, Feb 06, 2017 at 12:16:44AM +0900, Minchan Kim wrote:
>> +static inline void zram_fill_page(char *ptr, unsigned long len,
>> +					unsigned long value)
>> +{
>> +	int i;
>> +	unsigned long *page = (unsigned long *)ptr;
>> +
>> +	WARN_ON_ONCE(!IS_ALIGNED(len, sizeof(unsigned long)));
>> +
>> +	if (likely(value == 0)) {
>> +		memset(ptr, 0, len);
>> +	} else {
>> +		for (i = 0; i < len / sizeof(*page); i++)
>> +			page[i] = value;
>> +	}
>> +}
>
> I would suggest:
>
> #ifndef __HAVE_ARCH_MEMFILL
> /**
>  * memfill - Fill a region of memory with the given value
>  * @s: Pointer to the start of the region.
>  * @v: The word to fill the region with.
>  * @n: The size of the region.
>  *
>  * Differs from memset() in that it fills with an unsigned long instead of
>  * a byte.  The pointer and the size must be aligned to unsigned long.
>  */
> void memfill(unsigned long *s, unsigned long v, size_t n)
> {
> 	WARN_ON_ONCE(!IS_ALIGNED(n, sizeof(v)));
>
> 	if (likely(v == 0)) {
> 		memset(s, 0, n);
> 	} else {
> 		while (n) {
> 			*s++ = v;
> 			n -= sizeof(v);
> 		}
> 	}
> }
> EXPORT_SYMBOL(memfill);
> #endif
>
> (I would also suggest this move to lib/string.c and architectures be
> given the opportunity to provide an optimised version of memfill).
>

good idea, i hope kernel could support family functions like memfill/memset_long
in lib. but this is beyond zram scope.
>
> .
>

Re: memfill

[Minchan Kim]

Hi Matthew,

On Mon, Feb 06, 2017 at 06:49:02AM -0800, Matthew Wilcox wrote:
> 
> [adding linux-arch to see if anyone there wants to do an optimised
> version of memfill for their CPU]
> 
> On Mon, Feb 06, 2017 at 12:16:44AM +0900, Minchan Kim wrote:
> > +static inline void zram_fill_page(char *ptr, unsigned long len,
> > +					unsigned long value)
> > +{
> > +	int i;
> > +	unsigned long *page = (unsigned long *)ptr;
> > +
> > +	WARN_ON_ONCE(!IS_ALIGNED(len, sizeof(unsigned long)));
> > +
> > +	if (likely(value == 0)) {
> > +		memset(ptr, 0, len);
> > +	} else {
> > +		for (i = 0; i < len / sizeof(*page); i++)
> > +			page[i] = value;
> > +	}
> > +}
> 
> I would suggest:
> 
> #ifndef __HAVE_ARCH_MEMFILL
> /** 
>  * memfill - Fill a region of memory with the given value
>  * @s: Pointer to the start of the region.
>  * @v: The word to fill the region with.
>  * @n: The size of the region.
>  * 
>  * Differs from memset() in that it fills with an unsigned long instead of 
>  * a byte.  The pointer and the size must be aligned to unsigned long.
>  */
> void memfill(unsigned long *s, unsigned long v, size_t n)
> {
> 	WARN_ON_ONCE(!IS_ALIGNED(n, sizeof(v)));
> 
> 	if (likely(v == 0)) {
> 		memset(s, 0, n);
> 	} else {
> 		while (n) {
> 			*s++ = v;
> 			n -= sizeof(v);
> 		}
> 	}
> }
> EXPORT_SYMBOL(memfill);
> #endif
> 
> (I would also suggest this move to lib/string.c and architectures be
> given the opportunity to provide an optimised version of memfill).

Thanks for suggestion, Matthew!

However, I want to mention zram's performance wouldn't be sensitive
for that function because non-zero memfill would be rare.

Re: memfill

[David Howells]

Matthew Wilcox <willy@infradead.org> wrote:

> [adding linux-arch to see if anyone there wants to do an optimised
> version of memfill for their CPU]

For mn10300, this is superfluous since the memset() implementation will do
optimised filling of up to 8 x 4 bytes per loop if the alignments suit.

This is also superfluous for frv as that will do up to 8 x 8 bytes per loop.

So on both those arches, memfill() should probably just wrap memset().

David

Re: memfill

[David Howells]

Matthew Wilcox <willy@infradead.org> wrote:

> [adding linux-arch to see if anyone there wants to do an optimised
> version of memfill for their CPU]

For mn10300, this is superfluous since the memset() implementation will do
optimised filling of up to 8 x 4 bytes per loop if the alignments suit.

This is also superfluous for frv as that will do up to 8 x 8 bytes per loop.

So on both those arches, memfill() should probably just wrap memset().

David

Re: memfill

[Matthew Wilcox]

On Tue, Feb 07, 2017 at 09:40:04AM +0000, David Howells wrote:
> Matthew Wilcox <willy@infradead.org> wrote:
> 
> > [adding linux-arch to see if anyone there wants to do an optimised
> > version of memfill for their CPU]
> 
> For mn10300, this is superfluous since the memset() implementation will do
> optimised filling of up to 8 x 4 bytes per loop if the alignments suit.
> 
> This is also superfluous for frv as that will do up to 8 x 8 bytes per loop.
> 
> So on both those arches, memfill() should probably just wrap memset().

You've misunderstood the purpose of memfill.  memfill allows the caller
to specify a pattern which is not a single byte in size, eg memfill(addr,
0x12345678, 64) would result in 0x12345678 being reproduced 16 times.
memset(addr, 0x12345678, 64) would result in 0x78 being reproduced
64 times.

Re: memfill

[David Howells]

Matthew Wilcox <willy@infradead.org> wrote:

> You've misunderstood the purpose of memfill.  memfill allows the caller
> to specify a pattern which is not a single byte in size, eg memfill(addr,
> 0x12345678, 64) would result in 0x12345678 being reproduced 16 times.
> memset(addr, 0x12345678, 64) would result in 0x78 being reproduced
> 64 times.

Ah.  Should it take a unsigned int rather than an unsigned long?

David

Re: memfill

[Matthew Wilcox]

On Tue, Feb 07, 2017 at 05:29:15PM +0000, David Howells wrote:
> Matthew Wilcox <willy@infradead.org> wrote:
> 
> > You've misunderstood the purpose of memfill.  memfill allows the caller
> > to specify a pattern which is not a single byte in size, eg memfill(addr,
> > 0x12345678, 64) would result in 0x12345678 being reproduced 16 times.
> > memset(addr, 0x12345678, 64) would result in 0x78 being reproduced
> > 64 times.
> 
> Ah.  Should it take a unsigned int rather than an unsigned long?

Well, our one user so far is looking to use an unsigned long (indeed,
wouldn't be able to use it if it took an unsigned int).  If we have users
who want to memfill with an unsigned int, they can do something like:

void memfill_int(int *a, unsigned int v, size_t n)
{
	if ((unsigned long)a & (sizeof(unsigned long) - 1)) {
		*a++ = v;
		n -= sizeof(unsigned int);
	}
	memfill((unsigned long *)a, v | ((v << 16) << 16), n);
	if (n & (sizeof(unsigned long) - 1)
		a[n / sizeof(v)] = v;
}

... but since we know of no users today, I don't want to put that in.

Re: memfill

[Matthew Wilcox]

On Tue, Feb 07, 2017 at 05:29:15PM +0000, David Howells wrote:
> Matthew Wilcox <willy@infradead.org> wrote:
> 
> > You've misunderstood the purpose of memfill.  memfill allows the caller
> > to specify a pattern which is not a single byte in size, eg memfill(addr,
> > 0x12345678, 64) would result in 0x12345678 being reproduced 16 times.
> > memset(addr, 0x12345678, 64) would result in 0x78 being reproduced
> > 64 times.
> 
> Ah.  Should it take a unsigned int rather than an unsigned long?

Well, our one user so far is looking to use an unsigned long (indeed,
wouldn't be able to use it if it took an unsigned int).  If we have users
who want to memfill with an unsigned int, they can do something like:

void memfill_int(int *a, unsigned int v, size_t n)
{
	if ((unsigned long)a & (sizeof(unsigned long) - 1)) {
		*a++ = v;
		n -= sizeof(unsigned int);
	}
	memfill((unsigned long *)a, v | ((v << 16) << 16), n);
	if (n & (sizeof(unsigned long) - 1)
		a[n / sizeof(v)] = v;
}

... but since we know of no users today, I don't want to put that in.

Re: memfill

[James Bottomley]

On Mon, 2017-02-06 at 06:49 -0800, Matthew Wilcox wrote:
> [adding linux-arch to see if anyone there wants to do an optimised
> version of memfill for their CPU]
> 
> On Mon, Feb 06, 2017 at 12:16:44AM +0900, Minchan Kim wrote:
> > +static inline void zram_fill_page(char *ptr, unsigned long len,
> > +					unsigned long value)
> > +{
> > +	int i;
> > +	unsigned long *page = (unsigned long *)ptr;
> > +
> > +	WARN_ON_ONCE(!IS_ALIGNED(len, sizeof(unsigned long)));
> > +
> > +	if (likely(value == 0)) {
> > +		memset(ptr, 0, len);
> > +	} else {
> > +		for (i = 0; i < len / sizeof(*page); i++)
> > +			page[i] = value;
> > +	}
> > +}
> 
> I would suggest:
> 
> #ifndef __HAVE_ARCH_MEMFILL
> /** 
>  * memfill - Fill a region of memory with the given value
>  * @s: Pointer to the start of the region.
>  * @v: The word to fill the region with.
>  * @n: The size of the region.
>  * 
>  * Differs from memset() in that it fills with an unsigned long
> instead of 
>  * a byte.  The pointer and the size must be aligned to unsigned
> long.
>  */
> void memfill(unsigned long *s, unsigned long v, size_t n)

If we're going to do this, are you sure we wouldn't be wanting a string
fill instead of a memfill (because filling either by byte or long looks
a bit restrictive) assuming static strings that we can tell the compile
time size of, it would be easy for generic code to optimise.

James

Re: memfill

[Matthew Wilcox]

On Tue, Feb 07, 2017 at 11:07:34AM -0800, James Bottomley wrote:
> > /** 
> >  * memfill - Fill a region of memory with the given value
> >  * @s: Pointer to the start of the region.
> >  * @v: The word to fill the region with.
> >  * @n: The size of the region.
> >  * 
> >  * Differs from memset() in that it fills with an unsigned long
> > instead of 
> >  * a byte.  The pointer and the size must be aligned to unsigned
> > long.
> >  */
> > void memfill(unsigned long *s, unsigned long v, size_t n)
> 
> If we're going to do this, are you sure we wouldn't be wanting a string
> fill instead of a memfill (because filling either by byte or long looks
> a bit restrictive) assuming static strings that we can tell the compile
> time size of, it would be easy for generic code to optimise.

When you say 'string fill', do you mean this?

void memfill(void *dst, size_t dsz, void *src, size_t ssz)
{
	if (ssz == 1) {
		memset(dst, *(unsigned char *)src, dsz);
	} else if (ssz == sizeof(short)) {
		memset_s(dst, *(unsigned short *)src, dsz);
	} else if (ssz == sizeof(int)) {
		memset_i(dst, *(unsigned int *)src, dsz);
	} else if (ssz == sizeof(long)) {
		memset_l(dst, *(unsigned long *)src, dsz);
	} else {
		while (dsz >= ssz) {
			memcpy(dst, src, ssz);
			dsz -= ssz;
			dst += ssz;
		}
		if (dsz)
			memcpy(dst. src. dsz);
	}
}

(with memset_[sil] being obvious, I hope).  Possibly some variation on
this to optimise compile-time constant dsz.

I have no objection to that.  Indeed, it would match Lars Wirzenius'
memfill() definition (if not implementation) which makes me quite happy.

Re: memfill

[James Bottomley]

On Wed, 2017-02-08 at 10:04 -0800, Matthew Wilcox wrote:
> On Tue, Feb 07, 2017 at 11:07:34AM -0800, James Bottomley wrote:
> > > /** 
> > >  * memfill - Fill a region of memory with the given value
> > >  * @s: Pointer to the start of the region.
> > >  * @v: The word to fill the region with.
> > >  * @n: The size of the region.
> > >  * 
> > >  * Differs from memset() in that it fills with an unsigned long
> > > instead of 
> > >  * a byte.  The pointer and the size must be aligned to unsigned
> > > long.
> > >  */
> > > void memfill(unsigned long *s, unsigned long v, size_t n)
> > 
> > If we're going to do this, are you sure we wouldn't be wanting a
> > string
> > fill instead of a memfill (because filling either by byte or long
> > looks
> > a bit restrictive) assuming static strings that we can tell the
> > compile
> > time size of, it would be easy for generic code to optimise.
> 
> When you say 'string fill', do you mean this?
> 
> void memfill(void *dst, size_t dsz, void *src, size_t ssz)
> {
> 	if (ssz == 1) {
> 		memset(dst, *(unsigned char *)src, dsz);
> 	} else if (ssz == sizeof(short)) {
> 		memset_s(dst, *(unsigned short *)src, dsz);
> 	} else if (ssz == sizeof(int)) {
> 		memset_i(dst, *(unsigned int *)src, dsz);
> 	} else if (ssz == sizeof(long)) {
> 		memset_l(dst, *(unsigned long *)src, dsz);
> 	} else {
> 		while (dsz >= ssz) {
> 			memcpy(dst, src, ssz);
> 			dsz -= ssz;
> 			dst += ssz;
> 		}
> 		if (dsz)
> 			memcpy(dst. src. dsz);
> 	}
> }
> 
> (with memset_[sil] being obvious, I hope).  Possibly some variation 
> on this to optimise compile-time constant dsz.
> 
> I have no objection to that.  Indeed, it would match Lars Wirzenius'
> memfill() definition (if not implementation) which makes me quite 
> happy.

Yes, that's about it.  My only qualm looking at the proposal was if
memfill is genuinely useful to something why would it only want to fill
in units of sizeof(long).  On the other hand, we've been operating for
decades without it, so perhaps memset_l is the only use case?

James

Re: memfill

[Matthew Wilcox]

On Wed, Feb 08, 2017 at 01:01:08PM -0800, James Bottomley wrote:
> Yes, that's about it.  My only qualm looking at the proposal was if
> memfill is genuinely useful to something why would it only want to fill
> in units of sizeof(long).  On the other hand, we've been operating for
> decades without it, so perhaps memset_l is the only use case?

I suspect we've grown hundreds of unoptimised implementations of this all
over the kernel.  I mean, look at the attitude of the zram developers when
I suggested memfill: "this is beyond zram scope."  I think finding all of
these is beyond the abilities of grep.  maybe coccinelle could find some?

Instead I chose a driver at random that both you and I are familiar with,
sym53c8xx_2.  Oh, look, here's one:

        np->badlun_sa = cpu_to_scr(SCRIPTB_BA(np, resel_bad_lun));
        for (i = 0 ; i < 64 ; i++)      /* 64 luns/target, no less */
                np->badluntbl[i] = cpu_to_scr(vtobus(&np->badlun_sa));

and another one:

                for (i = 0 ; i < 64 ; i++)
                        tp->luntbl[i] = cpu_to_scr(vtobus(&np->badlun_sa));

and another:

        for (i = 0 ; i < SYM_CONF_MAX_TASK ; i++)
                lp->itlq_tbl[i] = cpu_to_scr(np->notask_ba);

I don't think any of these are performance path, but they're there.

Maybe SCSI drivers are unusual.  Let's try a random network driver, e1000e:

        /* Clear shadow ram */
        for (i = 0; i < nvm->word_size; i++) {
                dev_spec->shadow_ram[i].modified = false;
                dev_spec->shadow_ram[i].value = 0xFFFF;
        }

(three of those loops)

I mean, it's not going to bring the house down, but that I chose two
drivers more or less at random and found places where such an API could
be used indicates there may be more places this should be used.  And it
gives architectures a good place to plug in a performance optimisation
for zram rather than hiding it away in that funny old driver almost
nobody looks at.

Re: memfill

[Matthew Wilcox]

On Wed, Feb 08, 2017 at 01:01:08PM -0800, James Bottomley wrote:
> Yes, that's about it.  My only qualm looking at the proposal was if
> memfill is genuinely useful to something why would it only want to fill
> in units of sizeof(long).  On the other hand, we've been operating for
> decades without it, so perhaps memset_l is the only use case?

I suspect we've grown hundreds of unoptimised implementations of this all
over the kernel.  I mean, look at the attitude of the zram developers when
I suggested memfill: "this is beyond zram scope."  I think finding all of
these is beyond the abilities of grep.  maybe coccinelle could find some?

Instead I chose a driver at random that both you and I are familiar with,
sym53c8xx_2.  Oh, look, here's one:

        np->badlun_sa = cpu_to_scr(SCRIPTB_BA(np, resel_bad_lun));
        for (i = 0 ; i < 64 ; i++)      /* 64 luns/target, no less */
                np->badluntbl[i] = cpu_to_scr(vtobus(&np->badlun_sa));

and another one:

                for (i = 0 ; i < 64 ; i++)
                        tp->luntbl[i] = cpu_to_scr(vtobus(&np->badlun_sa));

and another:

        for (i = 0 ; i < SYM_CONF_MAX_TASK ; i++)
                lp->itlq_tbl[i] = cpu_to_scr(np->notask_ba);

I don't think any of these are performance path, but they're there.

Maybe SCSI drivers are unusual.  Let's try a random network driver, e1000e:

        /* Clear shadow ram */
        for (i = 0; i < nvm->word_size; i++) {
                dev_spec->shadow_ram[i].modified = false;
                dev_spec->shadow_ram[i].value = 0xFFFF;
        }

(three of those loops)

I mean, it's not going to bring the house down, but that I chose two
drivers more or less at random and found places where such an API could
be used indicates there may be more places this should be used.  And it
gives architectures a good place to plug in a performance optimisation
for zram rather than hiding it away in that funny old driver almost
nobody looks at.