RE: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast paths"

RE: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast paths"

[Tian, Kevin]

>From: Rusty Russell
>Sent: 2005Äê11ÔÂ21ÈÕ 12:53
>Hi all,
>
>	While browsing the code, I noticed this in xmalloc.c:
>
>#ifndef __ia64__
>    BUG_ON(align > SMP_CACHE_BYTES);
>#endif
>
>	This is clearly wrong: due to header alignment we cannot give you a
>greater alignment than SMP_CACHE_BYTES.  Overriding this will cause the
>allocation to succeed, but not give the alignment requested.  It usually
>indicates the caller should be fixed.
>
>Does someone with an ia64 box know why, or want to rip it out and see
>what breaks?
>
>Thanks!
>Rusty.
>--
>A bad analogy is like a leaky screwdriver -- Richard Braakman
>

I didn't note that ifdef before, and removing it doesn't break dom0. I doubt the reason from following definition which is copied from Linux directly into Xen:

#ifdef CONFIG_SMP
# define SMP_CACHE_SHIFT	L1_CACHE_SHIFT
# define SMP_CACHE_BYTES	L1_CACHE_BYTES
#else
  /*
   * The "aligned" directive can only _increase_ alignment, so this is
   * safe and provides an easy way to avoid wasting space on a
   * uni-processor:
   */
# define SMP_CACHE_SHIFT	3
# define SMP_CACHE_BYTES	(1 << 3)
#endif

For a UP system, SMP_CACHE_BYTES is only 8 bytes to save space. Then maybe several months ago, there's such requirement to xmalloc a 16bytes aligned (legal on ia64 system like a long double) structure and then failed due to BUG_ON with an immediate lazy hack to add ifdef there. For now, such case seems disappearing after a quick test.

However I think the preferred way is to increase SMP_CACHE_SHIFT to 4 to match maximum allowed alignment value. But I'm not sure from the comment above why "3" is defined there on native linux. CC to linux-ia64 list for standard answer. ;-)

Thanks,
Kevin

Re: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast paths"

[Luck, Tony]

This comment:
>   /*
>    * The "aligned" directive can only _increase_ alignment, so this is
>    * safe and provides an easy way to avoid wasting space on a
>    * uni-processor:
>    */
suggests that we only expected SMP_CACHE_BYTES to be used in "aligned"
directives, where having a smaller value than the actual alignment
requirement of some object would simply be ignored by the compiler.

A search for SMP_CACHE_BYTES across the kernel picks up a few drivers
that don't follow this usage model.   E.g. drivers/net/acenic.c might
print a warning about unsupported cache line size on a UP ia64.

Your usage:
	BUG_ON(align > SMP_CACHE_BYTES);

is another instance of this define being used in a way that ia64
didn't expect (it appears that only ia64 has this space saving
trick in the definition of SMP_CACHE_BYTES ... so it is unsurprising
that general users are not following our usage model).

How much is this trick saving us?  Static size of data area in
vmlinux doesn't change very much as SMP_CACHE_BYTES is varied:

   text    data     bss     dec     hex filename
8677481 1139704 1206357 11023542         a834b6 vmlinux-8
8677417 1141808 1206397 11025622         a83cd6 vmlinux-16
8677417 1146000 1206477 11029894         a84d86 vmlinux-32
8677353 1146256 1206573 11030182         a84ea6 vmlinux-64
8677353 1163152 1207085 11047590         a892a6 vmlinux-128

I'm not sure how to evaluate dynamic behavior (allocation of
structures whose size is dependent on SMP_CACHE_BYTES at
runtime).

-Tony

Re: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast paths"

[David Mosberger-Tang]

I don't think it's worth going to great lengths to preserve the
SMP_CACHE_BYTES = 8 for UP.  We should be able to just make it 16 and
be done with it (perhaps adding a comment that SMP_CACHE_BYTES must be
>= max alignment of any C type).

  --david

On 11/22/05, Luck, Tony <tony.luck@intel.com> wrote:
> This comment:
> >   /*
> >    * The "aligned" directive can only _increase_ alignment, so this is
> >    * safe and provides an easy way to avoid wasting space on a
> >    * uni-processor:
> >    */
> suggests that we only expected SMP_CACHE_BYTES to be used in "aligned"
> directives, where having a smaller value than the actual alignment
> requirement of some object would simply be ignored by the compiler.
>
> A search for SMP_CACHE_BYTES across the kernel picks up a few drivers
> that don't follow this usage model.   E.g. drivers/net/acenic.c might
> print a warning about unsupported cache line size on a UP ia64.
>
> Your usage:
>         BUG_ON(align > SMP_CACHE_BYTES);
>
> is another instance of this define being used in a way that ia64
> didn't expect (it appears that only ia64 has this space saving
> trick in the definition of SMP_CACHE_BYTES ... so it is unsurprising
> that general users are not following our usage model).
>
> How much is this trick saving us?  Static size of data area in
> vmlinux doesn't change very much as SMP_CACHE_BYTES is varied:
>
>    text    data     bss     dec     hex filename
> 8677481 1139704 1206357 11023542         a834b6 vmlinux-8
> 8677417 1141808 1206397 11025622         a83cd6 vmlinux-16
> 8677417 1146000 1206477 11029894         a84d86 vmlinux-32
> 8677353 1146256 1206573 11030182         a84ea6 vmlinux-64
> 8677353 1163152 1207085 11047590         a892a6 vmlinux-128
>
> I'm not sure how to evaluate dynamic behavior (allocation of
> structures whose size is dependent on SMP_CACHE_BYTES at
> runtime).
>
> -Tony
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


--
Mosberger Consulting LLC, voice/fax: 510-744-9372,
http://www.mosberger-consulting.com/
35706 Runckel Lane, Fremont, CA 94536

RE: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast paths"

[Tian, Kevin]

>From: Luck, Tony
>Sent: 2005Äê11ÔÂ23ÈÕ 0:11
>This comment:
>>   /*
>>    * The "aligned" directive can only _increase_ alignment, so this is
>>    * safe and provides an easy way to avoid wasting space on a
>>    * uni-processor:
>>    */
>suggests that we only expected SMP_CACHE_BYTES to be used in "aligned"
>directives, where having a smaller value than the actual alignment
>requirement of some object would simply be ignored by the compiler.

Understand. ;-)

>
>How much is this trick saving us?  Static size of data area in
>vmlinux doesn't change very much as SMP_CACHE_BYTES is varied:
>
>   text    data     bss     dec     hex filename
>8677481 1139704 1206357 11023542         a834b6 vmlinux-8
>8677417 1141808 1206397 11025622         a83cd6 vmlinux-16
>8677417 1146000 1206477 11029894         a84d86 vmlinux-32
>8677353 1146256 1206573 11030182         a84ea6 vmlinux-64
>8677353 1163152 1207085 11047590         a892a6 vmlinux-128
>
>I'm not sure how to evaluate dynamic behavior (allocation of
>structures whose size is dependent on SMP_CACHE_BYTES at
>runtime).
>
>-Tony
>-

So maybe we can simply remove the UP specific size definition and give both cases to actual cache line size.

Thanks,
Kevin

RE: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast paths"

[Tian, Kevin]

Now I think even '16' can't cover all cases. It's possible for a user defined structure with .align directive to force by '32' or larger, and then allocator happens to have similar check upon SMP_CACHE_BYTES like case in this thread. Because both structure definition and allocator may have no idea about IA64 trick of saving space for UP. Max alignment of any C style only solves the natural alignment case, but not above forced one. We can just give its real assumption to SMP_CACHE_BYTES - cache line size. ;-)

Thanks,
Kevin

>-----Original Message-----
>From: dmosberger@gmail.com [mailto:dmosberger@gmail.com] On Behalf Of David
>Mosberger-Tang
>Sent: 2005Äê11ÔÂ23ÈÕ 3:34
>To: Luck, Tony
>Cc: Tian, Kevin; Rusty Russell; Xen Mailing List; Tony Breeds; djm@kirby.fc.hp.com;
>linux-ia64@vger.kernel.org
>Subject: Re: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast
>paths"
>
>I don't think it's worth going to great lengths to preserve the
>SMP_CACHE_BYTES = 8 for UP.  We should be able to just make it 16 and
>be done with it (perhaps adding a comment that SMP_CACHE_BYTES must be
>>= max alignment of any C type).
>
>  --david
>
>On 11/22/05, Luck, Tony <tony.luck@intel.com> wrote:
>> This comment:
>> >   /*
>> >    * The "aligned" directive can only _increase_ alignment, so this is
>> >    * safe and provides an easy way to avoid wasting space on a
>> >    * uni-processor:
>> >    */
>> suggests that we only expected SMP_CACHE_BYTES to be used in "aligned"
>> directives, where having a smaller value than the actual alignment
>> requirement of some object would simply be ignored by the compiler.
>>
>> A search for SMP_CACHE_BYTES across the kernel picks up a few drivers
>> that don't follow this usage model.   E.g. drivers/net/acenic.c might
>> print a warning about unsupported cache line size on a UP ia64.
>>
>> Your usage:
>>         BUG_ON(align > SMP_CACHE_BYTES);
>>
>> is another instance of this define being used in a way that ia64
>> didn't expect (it appears that only ia64 has this space saving
>> trick in the definition of SMP_CACHE_BYTES ... so it is unsurprising
>> that general users are not following our usage model).
>>
>> How much is this trick saving us?  Static size of data area in
>> vmlinux doesn't change very much as SMP_CACHE_BYTES is varied:
>>
>>    text    data     bss     dec     hex filename
>> 8677481 1139704 1206357 11023542         a834b6 vmlinux-8
>> 8677417 1141808 1206397 11025622         a83cd6 vmlinux-16
>> 8677417 1146000 1206477 11029894         a84d86 vmlinux-32
>> 8677353 1146256 1206573 11030182         a84ea6 vmlinux-64
>> 8677353 1163152 1207085 11047590         a892a6 vmlinux-128
>>
>> I'm not sure how to evaluate dynamic behavior (allocation of
>> structures whose size is dependent on SMP_CACHE_BYTES at
>> runtime).
>>
>> -Tony
>> -
>> To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>
>--
>Mosberger Consulting LLC, voice/fax: 510-744-9372,
>http://www.mosberger-consulting.com/
>35706 Runckel Lane, Fremont, CA 94536

Re: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast paths"

[Keir Fraser]

On 23 Nov 2005, at 02:58, Tian, Kevin wrote:

> Now I think even '16' can't cover all cases. It's possible for a user 
> defined structure with .align directive to force by '32' or larger, 
> and then allocator happens to have similar check upon SMP_CACHE_BYTES 
> like case in this thread. Because both structure definition and 
> allocator may have no idea about IA64 trick of saving space for UP. 
> Max alignment of any C style only solves the natural alignment case, 
> but not above forced one. We can just give its real assumption to 
> SMP_CACHE_BYTES - cache line size. ;-)

It's not hard to support arbitrary alignment, at the cost of burning 
some space. We should probably do that.

  -- Keir

Re: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast paths"

[David Mosberger-Tang]

On 11/22/05, Tian, Kevin <kevin.tian@intel.com> wrote:
> Now I think even '16' can't cover all cases. It's possible for a user defined structure with .align directive to force by '32' or larger, and then allocator happens to have similar check upon SMP_CACHE_BYTES like case in this thread. Because both structure definition and allocator may have no idea about IA64 trick of saving space for UP. Max alignment of any C style only solves the natural alignment case, but not above forced one. We can just give its real assumption to SMP_CACHE_BYTES - cache line size. ;-)

Not likely to be a problem.  Memory allocators generally guarantee
only 16-byte alignment (as per ia64 software conventions).

  --david
--
Mosberger Consulting LLC, voice/fax: 510-744-9372,
http://www.mosberger-consulting.com/
35706 Runckel Lane, Fremont, CA 94536

RE: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast paths"

[Luck, Tony]

>It's not hard to support arbitrary alignment, at the cost of burning 
>some space. We should probably do that.

The "we" in that last sentence is the Xen team ... referring
to making fixes to xmalloc?

-Tony

Re: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast paths"

[Keir Fraser]

On 23 Nov 2005, at 15:07, Luck, Tony wrote:

>> It's not hard to support arbitrary alignment, at the cost of burning
>> some space. We should probably do that.
>
> The "we" in that last sentence is the Xen team ... referring
> to making fixes to xmalloc?

Correct. But I've thought more on it and I guess that actually the 
number of cases where we have structures with alignment requirements 
stricter than SMP_CACHE_BYTES will be very small. In fact I can't think 
of any in Xen right now. :-)

So it makes most sense for ia64 Xen to define SMP_CACHE_BYTES to a 
sensible largeish number irrespective of CONFIG_SMP (after all, how 
many uniproc ia64 systems are there), and solve the general alignment 
problem in xmalloc only if we really need to.

  -- Keir

Re: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling

[Rusty Russell]

On Wed, 2005-11-23 at 15:37 +0000, Keir Fraser wrote:
> On 23 Nov 2005, at 15:07, Luck, Tony wrote:
> 
> >> It's not hard to support arbitrary alignment, at the cost of burning
> >> some space. We should probably do that.
> >
> > The "we" in that last sentence is the Xen team ... referring
> > to making fixes to xmalloc?
> 
> Correct. But I've thought more on it and I guess that actually the 
> number of cases where we have structures with alignment requirements 
> stricter than SMP_CACHE_BYTES will be very small. In fact I can't think 
> of any in Xen right now. :-)

Right, which was why the original BUG_ON() which started this
discussion...

Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

RE: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast paths"

[Magenheimer, Dan (HP Labs Fort Collins)]

> Subject: RE: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix 
> ar.unat handling forfast paths"
> 
> >It's not hard to support arbitrary alignment, at the cost of burning 
> >some space. We should probably do that.
> 
> The "we" in that last sentence is the Xen team ... referring
> to making fixes to xmalloc?
> 
> -Tony

Since this thread is cross-posted to two lists neither one of which
is xen-ia64-devel, a short explanation might be in order:

Since there a fair overlap of functionality, Xen/ia64 heavily
leverages Linux and Linux/ia64 code.  About 120 .c/.h files are
100% reused from linux-2.6.13 and another 36 files are
reused but require small patches (total of ~120 chunks, mostly
1-2 line diffs).  The leveraged files are periodically updated
to use the latest version of linux (started with 2.6.7, then 2.6.9,
now 2.6.13); this allows Xen/ia64 to easily pick up new functionality
and bug fixes from Linux/ia64 without reinventing the wheel.

HOWEVER, one disadvantage is that some bugs in Linux get inherited
by Xen/ia64.  As Tony previously noted, Linux/ia64 makes some
assumptions about SMP_CACHE_BYTES which are not adhered to throughout
Linux.  Xen apparently does not adhere to those assumptions either.
Xen/ia64 will need to add another patch to the (previously unchanged)
file linux/include/asm-ia64/cache.h.  Presumably this should
be fixed in Linux/ia64 as well and the file will be resynced
in the future.

Hope that helps!

Dan

RE: [Xen-devel] __ia64__ ifdef in xmalloc.c: "Fix ar.unat handling forfast paths"

[Luck, Tony]

>So it makes most sense for ia64 Xen to define SMP_CACHE_BYTES to a 
>sensible largeish number irrespective of CONFIG_SMP (after all, how 
>many uniproc ia64 systems are there), and solve the general alignment 
>problem in xmalloc only if we really need to.

Changing from 8 to 16 (as David suggested) looks to be the right
thing to do.  If you think it needs to be higher than this, then
I'll need some more convincing.

-Tony