[PATCH] (2.4.x bk) efi_memmap_walk_uc

[PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christopher Wedgwood]

Hi,

On SN2 we have a 'fetchop' driver which does various magical things
with uncached memory (atomic operations within or across partitions).

To support this we have efi_memmap_walk_uc defined in a very similar
way to efi_memmap_walk.

How do people feel about merging this in the generic code (rather than
hiding it away in the driver or something more complex like
abstraction and changes to the current efi code)?

I'd also like to see a version of this in 2.5.x.


 arch/ia64/kernel/efi.c |   27 +++++++++++++++++++++++++++
 include/linux/efi.h    |    1 +
 2 files changed, 28 insertions(+)



Flame away,

  --cw


=== arch/ia64/kernel/efi.c 1.12 vs edited ==--- 1.12/arch/ia64/kernel/efi.c	Fri Mar 14 16:08:01 2003
+++ edited/arch/ia64/kernel/efi.c	Mon Jul 21 17:34:27 2003
@@ -723,3 +723,30 @@
  	remove_proc_entry(efi_dir->name, NULL);
 #endif
 }
+
+/*
+ * Walks the EFI memory map and calls CALLBACK once for each EFI memory descriptor that
+ * has memory that marked as only EFI_MEMORY_UC.
+ */
+void
+efi_memmap_walk_uc (efi_freemem_callback_t callback, void *arg)
+{
+	void *efi_map_start, *efi_map_end, *p;
+	efi_memory_desc_t *md;
+	u64 efi_desc_size, start, end;
+
+	efi_map_start = __va(ia64_boot_param->efi_memmap);
+	efi_map_end   = efi_map_start + ia64_boot_param->efi_memmap_size;
+	efi_desc_size = ia64_boot_param->efi_memdesc_size;
+
+	for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) {
+		md = p;
+
+		if (md->attribute = EFI_MEMORY_UC) {
+			start = PAGE_ALIGN(md->phys_addr);
+			end = PAGE_ALIGN((md->phys_addr+(md->num_pages << EFI_PAGE_SHIFT)) & PAGE_MASK);
+			if ((*callback)(start, end, arg) < 0)
+				return;
+		}
+	}
+}
=== include/linux/efi.h 1.3 vs edited ==--- 1.3/include/linux/efi.h	Thu Sep 12 11:57:59 2002
+++ edited/include/linux/efi.h	Mon Jul 21 17:35:43 2003
@@ -260,6 +260,7 @@
 extern void efi_init (void);
 extern void efi_map_pal_code (void);
 extern void efi_memmap_walk (efi_freemem_callback_t callback, void *arg);
+extern void efi_memmap_walk_uc (efi_freemem_callback_t callback, void *arg);
 extern void efi_gettimeofday (struct timeval *tv);
 extern void efi_enter_virtual_mode (void);	/* switch EFI to virtual mode, if possible */
 extern u64 efi_get_iobase (void);

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Bjorn Helgaas]

On Monday 21 July 2003 6:43 pm, Christopher Wedgwood wrote:
> To support this we have efi_memmap_walk_uc defined in a very similar
> way to efi_memmap_walk.

I sort of object to adding a parallel function just to extract
regions with a different attribute.  I'd rather rework efi_memmap_walk
to call the callback for *every* memory descriptor, and make the
callback responsible for filtering out the regions of interest.

The only problem is that the existing callbacks would have
to be changed.  Plus, all the trimming that's currently in
efi_memmap_walk to make sure all the granules with WB
memory are completely WB, would have to be reworked
or pulled out into its own function.  I kind of like the idea
of factoring it out anyway.

Bjorn

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christopher Wedgwood]

On Mon, Jul 21, 2003 at 08:16:04PM -0600, Bjorn Helgaas wrote:

> I sort of object to adding a parallel function just to extract
> regions with a different attribute.

As do I.

> I'd rather rework efi_memmap_walk to call the callback for *every*
> memory descriptor, and make the callback responsible for filtering
> out the regions of interest.

I'd prefer we cleaned up the entire EFI code to do this and a few
other things differently --- however, given the status of 2.4.x I'm
not sure this is desirable?

> The only problem is that the existing callbacks would have to be
> changed.  Plus, all the trimming that's currently in efi_memmap_walk
> to make sure all the granules with WB memory are completely WB,
> would have to be reworked or pulled out into its own function.  I
> kind of like the idea of factoring it out anyway.

Given that is this acceptable for 2.4.x as-is or should I move the
code into the driver as distasteful as that is.

I'm not convinced that massive EFI API changes right now are a good
idea (not to mention I certainly have no time for them myself).



Thanks,
  --cw

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Bjorn Helgaas]

> I'd prefer we cleaned up the entire EFI code to do this and a few
> other things differently --- however, given the status of 2.4.x I'm
> not sure this is desirable?

Yeah, I agree.  What about doing it for 2.5/2.6 first?

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christopher Wedgwood]

On Mon, Jul 21, 2003 at 09:08:28PM -0600, Bjorn Helgaas wrote:

> Yeah, I agree.  What about doing it for 2.5/2.6 first?

If David is willing I'll take a stab...  the code is pretty horrendous
in places in the way it runs on large NUMA machines from memory
(printk efi_memmap_walk and watch the run).

I'll make some notes later this week for people to tear apart.

For 2.4.x though I'll move the code into the driver as I have pretty
limited cycles.



Thanks,
  --cw

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[David Mosberger]

>>>>> On Mon, 21 Jul 2003 17:43:54 -0700, Christopher Wedgwood <cw@sgi.com> said:

  Chris> Hi, On SN2 we have a 'fetchop' driver which does various
  Chris> magical things with uncached memory (atomic operations within
  Chris> or across partitions).

  Chris> To support this we have efi_memmap_walk_uc defined in a very
  Chris> similar way to efi_memmap_walk.

  Chris> How do people feel about merging this in the generic code
  Chris> (rather than hiding it away in the driver or something more
  Chris> complex like abstraction and changes to the current efi
  Chris> code)?

  Chris> I'd also like to see a version of this in 2.5.x.

I really would like to see the code that's using efi_memmap_walk_uc()
first.

Also, a check like this:

  Chris> if (md->attribute = EFI_MEMORY_UC)

is almost certainly wrong (md->attribute is a bitmap).

	--david

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christopher Wedgwood]

On Thu, Jul 24, 2003 at 06:15:42PM -0700, David Mosberger wrote:

> I really would like to see the code that's using
> efi_memmap_walk_uc() first.

I think Jack sent this to you already, but for the benefit of the
list (relevant functions rather than send to the list the entire
driver):

/*
 * fetchop_build_memmap,
 *
 * Called at boot time to build a map of pages that can be used for
 * fetchops.
 */
static int __init
fetchop_build_memmap(unsigned long start, unsigned long end, void *arg)
{
	struct node_fetchops *fops;
	long count, bytes;

	count = (end - start) >> PAGE_SHIFT;
	bytes = sizeof(struct node_fetchops) + count/8;
	fops = vmalloc(bytes);
	memset(fops, 0, bytes);
	fops->maddr = FETCHOP_KADDR_TO_MSPEC_ADDR(start);
	fops->count = count;
	atomic_add(count, &fops->free);
	fetchop_stats.pages_total += count;
	node_fetchops[MSPEC_TO_NID(start)] = fops;

	sn_flush_all_caches((long)__va(start), end - start);

	return 0;
}

/*
 * fetchop_init
 *
 * Called at boot time to initialize the fetchop facility.
 */
int __init
fetchop_init(void)
{
	[...]
	efi_memmap_walk_uc(fetchop_build_memmap, 0);
	printk(KERN_INFO "%s: v%s\n", DRIVER_ID_STR, REVISION);

	return 0;
}


> Also, a check like this:
>
>   Chris> if (md->attribute = EFI_MEMORY_UC)
>
> is almost certainly wrong (md->attribute is a bitmap).

I think the logic here seems to have been uncached ONLY (ie. all other
bits zero).  I guess this falls down if new attributes are added so
something like:

      if ((md->attribute & SOME_MASK) = EFI_MEMORY_UC)
	 [...]

would then be required.  Since this is the only consumer of this
interface thus far I'm not sure it's obvious how the function should
best work.



The issue right now seems to be whether or not this or some variation
of this is useful and/or desirable in the mainline kernel or whether
for now it's best to hide this in the driver until a later stage when
the EFI interfaces are abstracted out a little more to make this
easier?



Thanks,
  --cw

RE: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Luck, Tony]

> would then be required.  Since this is the only consumer of this
> interface thus far I'm not sure it's obvious how the function should
> best work.
> 
> 
> 
> The issue right now seems to be whether or not this or some variation
> of this is useful and/or desirable in the mainline kernel or whether
> for now it's best to hide this in the driver until a later stage when
> the EFI interfaces are abstracted out a little more to make this
> easier?

Even though you are the only user ... it would seem prudent
to define some generic interface.  Right now you just assume
(correctly) that all the uncacheable memory has been ignored
by Linux.  There are two problems with this:

1) Someday that might change. E.g. Linux might need to alloc uncacheable
   memory for min_state areas for more complex MCA recovery

2) There is no way for another non-kernel user of uncacheable
   memory to share the resource with you.

-Tony

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christopher Wedgwood]

On Tue, Jul 29, 2003 at 11:45:40AM -0700, Luck, Tony wrote:

> Even though you are the only user ... it would seem prudent
> to define some generic interface.

I agree but I'm more inclined to think that should be part of an EFI
interfaces cleanup though and more medium to long term perhaps?  I'd
rather see a small limited interface initially and then have that
'fixed' as requirements become more obvious.

> 2) There is no way for another non-kernel user of uncacheable
>    memory to share the resource with you.

The current fetchop driver export interfaces:

    /*-----------------------------------------------------------------------------
     * KERNEL APIs
     * 	Note: right now, these APIs return a full page of fetchops.  If these
     *	interfaces are used often for tasks which do not require a full page of
     *	fetchops, new APIs should be added to suballocate out of a single page.
     */

    unsigned long
    fetchop_kalloc_page(int nid)
    {
	    if (fetchop_update_stats(1, 1) < 0)
		    return 0;
	    return fetchop_alloc_page(nid);
    }
    EXPORT_SYMBOL(fetchop_kalloc_page);


    void
    fetchop_kfree_page(unsigned long maddr)
    {
	    fetchop_free_page(maddr);
	    fetchop_update_stats(-1, -1);
    }
    EXPORT_SYMBOL(fetchop_kfree_page);

Might these be more generically useful at some later stage?


Thanks,
  --cw

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[David Mosberger]

>>>>> On Tue, 29 Jul 2003 11:34:07 -0700, Christopher Wedgwood <cw@sgi.com> said:

  Christopher> On Thu, Jul 24, 2003 at 06:15:42PM -0700, David Mosberger wrote:

  >> I really would like to see the code that's using
  >> efi_memmap_walk_uc() first.

  Christopher> I think Jack sent this to you already

If he did, I don't remember (and I don't have it in my inbox).

  >> Also, a check like this:

  Chris> if (md->attribute = EFI_MEMORY_UC)

  >> is almost certainly wrong (md->attribute is a bitmap).

  Christopher> I think the logic here seems to have been uncached ONLY
  Christopher> (ie. all other bits zero).

There are many bits in attribute, such as MEMORY_RUNTIME etc.  The
real problem is that the code you sent me makes little sense to me.
It creates a "node_fetchops" structure for _every_ uncached block of
address space?  Will there be problems if that address space/memory is
used for other purposes?

  Christopher> I guess this falls down if new attributes are added so
  Christopher> something like:

  Christopher> if ((md->attribute & SOME_MASK) = EFI_MEMORY_UC)
  Christopher> [...]

  Christopher> would then be required.  Since this is the only
  Christopher> consumer of this interface thus far I'm not sure it's
  Christopher> obvious how the function should best work.

I think what we really want here is two bitmasks: one telling us which
bits we care about and one telling us what values those bits should be
set to.  As it stands, efi_memmap_walk_uc() doesn't do what it
advertises to do and instead does something very specific to SN2.

  Christopher> The issue right now seems to be whether or not this or
  Christopher> some variation of this is useful and/or desirable in
  Christopher> the mainline kernel or whether for now it's best to
  Christopher> hide this in the driver until a later stage when the
  Christopher> EFI interfaces are abstracted out a little more to make
  Christopher> this easier?

Can you work with Tony on this?  Since he's Mr. MCA (you OK with this
title, Tony? ;-) and since we'll have to map the MINSTATE area UC, I
think we want to make sure that the interface can handle all of this.

	--david

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christopher Wedgwood]

On Tue, Jul 29, 2003 at 01:41:51PM -0700, David Mosberger wrote:

> There are many bits in attribute, such as MEMORY_RUNTIME etc.  The
> real problem is that the code you sent me makes little sense to me.

I can send the entire driver if that helps?  I think strictly speaking
the issue is orthogonal to any specific driver if it's to be included
outside the driver or platform code which why I brought this up.

> It creates a "node_fetchops" structure for _every_ uncached block of
> address space?  Will there be problems if that address space/memory
> is used for other purposes?

No --- because there are no other users...  the code was almost
certainly written a long time ago to solve an SN specific purpose and
has been carried around in the form I presented.

I fully admit that in a general sense this code isn't workable but I
think perhaps the concept of some kind of UC allocator might be.  If
such a thing existed I'm quite sure we would have the fetchop driver
ported to use it.

> I think what we really want here is two bitmasks: one telling us
> which bits we care about and one telling us what values those bits
> should be set to.

Yes.  But does that really make the code more suitable for merging or
is that merely a small step towards the larger picture?

> Can you work with Tony on this?  Since he's Mr. MCA (you OK with
> this title, Tony? ;-) and since we'll have to map the MINSTATE area
> UC, I think we want to make sure that the interface can handle all
> of this.

Sure, I'm happy with that.  I had considered as for UC we could try to
encourage everyone to have the MINSTATE containing granule to be all
UC and perhaps for generic kernels compromise on 16MB granules?
That's a different issue entirely though.



Thanks,
  --cw

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[David Mosberger]

>>>>> On Tue, 29 Jul 2003 14:15:20 -0700, Christopher Wedgwood <cw@sgi.com> said:

  Christopher> No --- because there are no other users...  the code
  Christopher> was almost certainly written a long time ago to solve
  Christopher> an SN specific purpose and has been carried around in
  Christopher> the form I presented.

  Christopher> I fully admit that in a general sense this code isn't
  Christopher> workable but I think perhaps the concept of some kind
  Christopher> of UC allocator might be.

I'm sorry, but that's _exactly_ the problem.  You are pushing an
SN2-specific hacks into the ia64 kernel and you seem to think that's
OK.  I don't want the ia64 kernel to turn into a collection of
vendor-specific hacks (whether from HP, Intel, SGI, or anyone else).
Yes, doing clean, general and efficient APIs is hard, requires
thinking, talking to others, etc., but it's the only way to get a
kernel that's maintainable.  If you only care about
efi_memmap_walk_uc() for SN2, that's fine by me, but in that case it
will stay in the SN2 tree.  If you want something in the normal ia64
kernel, please propose an API (and patch) that actually makes sense
beyond SN2.

	--david

RE: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Luck, Tony]

> > It creates a "node_fetchops" structure for _every_ uncached block of
> > address space?  Will there be problems if that address space/memory
> > is used for other purposes?
> 
> No --- because there are no other users...  the code was almost
> certainly written a long time ago to solve an SN specific purpose and
> has been carried around in the form I presented.
> 
> I fully admit that in a general sense this code isn't workable but I
> think perhaps the concept of some kind of UC allocator might be.  If
> such a thing existed I'm quite sure we would have the fetchop driver
> ported to use it.

I definitely like the idea of a UC allocator that lives below the
level of the fetchop driver that can hand it whatever pieces of
uncacheable memory that it needs.  When MCA eventually gets sophisticated
enough to need to allocate more min_state areas, there will be a second
user ... and I hope that we are too far away from being there.

Initially the UC allocator should be really, really basic.  Perhaps
just handing out pages, and without any "free" code to complicate it
(MCA code should only need to allocate an alternate min_state area for
each cpu to handle returning to a different context from the one that
was running when the fault was reported).

> > I think what we really want here is two bitmasks: one telling us
> > which bits we care about and one telling us what values those bits
> > should be set to.
> 
> Yes.  But does that really make the code more suitable for merging or
> is that merely a small step towards the larger picture?
> 
> > Can you work with Tony on this?  Since he's Mr. MCA (you OK with
> > this title, Tony? ;-) and since we'll have to map the MINSTATE area
> > UC, I think we want to make sure that the interface can handle all
> > of this.
> 
> Sure, I'm happy with that.  I had considered as for UC we could try to
> encourage everyone to have the MINSTATE containing granule to be all
> UC and perhaps for generic kernels compromise on 16MB granules?
> That's a different issue entirely though.

-Tony

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Jack Steiner]

> 
> >>>>> On Tue, 29 Jul 2003 14:15:20 -0700, Christopher Wedgwood <cw@sgi.com> said:
> 
>   Christopher> No --- because there are no other users...  the code
>   Christopher> was almost certainly written a long time ago to solve
>   Christopher> an SN specific purpose and has been carried around in
>   Christopher> the form I presented.
> 
>   Christopher> I fully admit that in a general sense this code isn't
>   Christopher> workable but I think perhaps the concept of some kind
>   Christopher> of UC allocator might be.
 
I've gotten a little confused about this thread. A general purpose UC allocator
is a lot more than is needed by SGI. All we need is a way to walk the memmap
& locate entries with a specific set of atributes (we need UC but
other users may have different requirements).

I think someone (david?) proposed an interface that would take a mask/attribute
pair. This is all that is needed. It seems general enough so that
other code may use it too. Would this be acceptible??


> I'm sorry, but that's _exactly_ the problem.  You are pushing an
> SN2-specific hacks into the ia64 kernel and you seem to think that's
> OK.  I don't want the ia64 kernel to turn into a collection of
> vendor-specific hacks (whether from HP, Intel, SGI, or anyone else).
> Yes, doing clean, general and efficient APIs is hard, requires
> thinking, talking to others, etc., but it's the only way to get a
> kernel that's maintainable.  If you only care about
> efi_memmap_walk_uc() for SN2, that's fine by me, but in that case it
> will stay in the SN2 tree.  If you want something in the normal ia64
> kernel, please propose an API (and patch) that actually makes sense
> beyond SN2.
> 
> 	--david


-- 
Thanks

Jack Steiner    (651-683-5302)   (vnet 233-5302)      steiner@sgi.com

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christopher Wedgwood]

On Tue, Jul 29, 2003 at 02:31:58PM -0700, David Mosberger wrote:
> I'm sorry, but that's _exactly_ the problem.  You are pushing an
> SN2-specific hacks into the ia64 kernel and you seem to think that's
> OK.

Right now I'm more than happy to put the current code into the driver.
Going forward though a more generic solution would be nice.

> If you want something in the normal ia64 kernel, please propose an
> API (and patch) that actually makes sense beyond SN2.

Sure thing.  I'm not bothered for 2.4.x (the driver can carry the
required allocator) but for 2.6.x hopefully we can get something
sorted.


On Tue, Jul 29, 2003 at 02:34:53PM -0700, Luck, Tony wrote:
> Initially the UC allocator should be really, really basic.  Perhaps
> just handing out pages, and without any "free" code to complicate it
> (MCA code should only need to allocate an alternate min_state area
> for each cpu to handle returning to a different context from the one
> that was running when the fault was reported).

Page granularity would be fine here.  I'm not sure anything more
complex would be required any time soon if ever.

We could like to be able to allocate from or near to a given node in
NUMA configurations.  When allocating pages for MINSTATE areas you can
then have that code try to allocate memory on the same node as the
appropriate CPU.

How about something like:

    void* ia64_alloc_uc_page(int nodeid);
    void ia64_free_us_page(void*);

as a starting point (for non-NUMA the nodeid won't be interesting of
course).

Ideally freeing memory shouldn't be that hard and for fetchop
certainly would be desirable.  We could probably get by with a naive
bitmap allocator per-node or even one of wli's alternative bootmem
allocator suggestions (massaged to suit).



  --cw

RE: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Luck, Tony]

> On Tue, Jul 29, 2003 at 02:34:53PM -0700, Luck, Tony wrote:
> > Initially the UC allocator should be really, really basic.  Perhaps
> > just handing out pages, and without any "free" code to complicate it
> > (MCA code should only need to allocate an alternate min_state area
> > for each cpu to handle returning to a different context from the one
> > that was running when the fault was reported).
> 
> Page granularity would be fine here.  I'm not sure anything more
> complex would be required any time soon if ever.
> 
> We could like to be able to allocate from or near to a given node in
> NUMA configurations.  When allocating pages for MINSTATE areas you can
> then have that code try to allocate memory on the same node as the
> appropriate CPU.

We can do that ... but i hope that MCA error recovery isn't a common
enough path that allocating min_state from the correct node ever shows
up on anyone's performance radar!

> How about something like:
> 
>     void* ia64_alloc_uc_page(int nodeid);
>     void ia64_free_us_page(void*);
> 
> as a starting point (for non-NUMA the nodeid won't be interesting of
> course).

Looks ok.  Is the return value from alloc a physical address or a
region 6 virtual address?  I don't think that I care, so you can
pick whatever works best for fetchop (and if I later do care, then it's
my own fault for not planning ahead :-)

> Ideally freeing memory shouldn't be that hard and for fetchop
> certainly would be desirable.  We could probably get by with a naive
> bitmap allocator per-node or even one of wli's alternative bootmem
> allocator suggestions (massaged to suit).

Last wli alternative bootmem allocator that I looked at looked like
overkill for this (it had all sorts of balanced trees for fast insert
and delete).  Is fetchop likely to alloc/free a lot (i.e.
is performance of alloc/free critical)?  A simple list of <base,length>
pairs (or maybe <node,base,length> triples??) would work (unless you
are expecting hundreds of blocks of uncacheable memory, and a high
alloc/free rate.

-Tony

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[David Mosberger]

>>>>> On Tue, 29 Jul 2003 15:07:01 -0700, Christopher Wedgwood <cw@sgi.com> said:

  Christopher> How about something like:

  Christopher> void* ia64_alloc_uc_page(int nodeid);
  Christopher> void ia64_free_us_page(void*);

  Christopher> as a starting point (for non-NUMA the nodeid won't be
  Christopher> interesting of course).

So the fetchop driver needs regular memory mapped as UC, just like the
MCA MINSTATE area?  If so, a UC memory allocator is certainly the way
to go.

	--david

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christopher Wedgwood]

On Tue, Jul 29, 2003 at 03:26:05PM -0700, Luck, Tony wrote:

> We can do that ... but i hope that MCA error recovery isn't a common
> enough path that allocating min_state from the correct node ever
> shows up on anyone's performance radar!

True.

> Looks ok.  Is the return value from alloc a physical address or a
> region 6 virtual address?

I guess strictly speaking it should be a u64 for pa.  We don't use a
specific type for physical addresses elsewhere do we?

> I don't think that I care, so you can pick whatever works best for
> fetchop (and if I later do care, then it's my own fault for not
> planning ahead :-)

I think we should just define pa since that's the units of the EFI
memory map...  and also define the minimum allocation granularity to
be EFI_PAGE_SIZE for the same reason.

> Last wli alternative bootmem allocator that I looked at looked like
> overkill for this (it had all sorts of balanced trees for fast
> insert and delete).

Heh, that is over kill.  I meant the one before that :)

> Is fetchop likely to alloc/free a lot (i.e.  is performance of
> alloc/free critical)?

I'm not sure how often it will free and/or alloc.  I'm not sure it
should be performance critical though so long as alloc/free work
reliably.

> A simple list of <base,length> pairs (or maybe <node,base,length>
> triples??) would work (unless you are expecting hundreds of blocks
> of uncacheable memory, and a high alloc/free rate.

That sounds pretty good.  Even a naive bitmap allocator would probably
work initially.  Might we ever want to allocate 2+ pages?  Forcing all
allocations to be a single page makes many things easier.


  --cw

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christopher Wedgwood]

On Tue, Jul 29, 2003 at 03:30:45PM -0700, David Mosberger wrote:

> So the fetchop driver needs regular memory mapped as UC, just like
> the MCA MINSTATE area?

Yes, but it's not used directly as if it were (however that detail is
a matter for the fetchop driver internally).

> If so, a UC memory allocator is certainly the way to go.

It sounds like it.


  --cw

RE: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Luck, Tony]

> That sounds pretty good.  Even a naive bitmap allocator would probably
> work initially.  Might we ever want to allocate 2+ pages?  Forcing all
> allocations to be a single page makes many things easier.

min_state areas for MCA will be smaller than a page ... so *my* use
of this allocator doesn't require multi-page.

You know best whether fetchop() will ever want more than a page.

So far, nobody else has leapt out of the woods to claim any other
uses for UC memory (that's a cue for anyone waiting to jump into this
discussion).

We could plan ahead for hypothetical future multi-page users by
defining a "size" argument to alloc/free, but keep our (your) life
simple for now by only implementing the one-page-at-a-time usage
model.

-Tony

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[David Mosberger]

>>>>> On Tue, 29 Jul 2003 15:35:16 -0700, Christopher Wedgwood <cw@sgi.com> said:

  Christopher> That sounds pretty good.  Even a naive bitmap allocator
  Christopher> would probably work initially.  Might we ever want to
  Christopher> allocate 2+ pages?  Forcing all allocations to be a
  Christopher> single page makes many things easier.

The allocation granularity should be bytes, in my opinion.  The point
of the UC allocator is so that we can share a single 64MB UC granule
across the multiple users (of which there are a grand total of 2, at
the moment...).

	--david

RE: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[David Mosberger]

>>>>> On Tue, 29 Jul 2003 15:47:34 -0700, "Luck, Tony" <tony.luck@intel.com> said:

  Tony> We could plan ahead for hypothetical future multi-page users by
  Tony> defining a "size" argument to alloc/free

Let's stick with a tried-and-true malloc/free-like interface (nobody
every gets size arguments to free() right, do they?).

  Tony> but keep our (your) life simple for now by only implementing
  Tony> the one-page-at-a-time usage model.

Sure, that sounds reasonable.

	--david

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Jack Steiner]

> > 
> > We could like to be able to allocate from or near to a given node in
> > NUMA configurations.  When allocating pages for MINSTATE areas you can
> > then have that code try to allocate memory on the same node as the
> > appropriate CPU.
> 
> We can do that ... but i hope that MCA error recovery isn't a common
> enough path that allocating min_state from the correct node ever shows
> up on anyone's performance radar!
 
True, but on NUMA systems (at least ours) you want MINSTATE area to be node
local for other reasons. Some types of errors cause the interconnect to fail.
The PAL/SAL should be able to log errors without making offnode references.




-- 
Thanks

Jack Steiner    (651-683-5302)   (vnet 233-5302)      steiner@sgi.com

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christopher Wedgwood]

On Tue, Jul 29, 2003 at 06:49:04PM -0500, Jack Steiner wrote:

> True, but on NUMA systems (at least ours) you want MINSTATE area to
> be node local for other reasons. Some types of errors cause the
> interconnect to fail.

I think that's a slightly different issue but related.  Being able to
avoid any platform specific code here would be good thing and should
be possible if we are given a cpu/nodeid in the allocator.

> The PAL/SAL should be able to log errors without making offnode
> references.

The question then is are these areas as used by the kernel going to be
referenced globally or only on a cpu/node local sense?  Tony?


  --cw

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Jack Steiner]

> 
> >>>>> On Tue, 29 Jul 2003 15:35:16 -0700, Christopher Wedgwood <cw@sgi.com> said:
> 
>   Christopher> That sounds pretty good.  Even a naive bitmap allocator
>   Christopher> would probably work initially.  Might we ever want to
>   Christopher> allocate 2+ pages?  Forcing all allocations to be a
>   Christopher> single page makes many things easier.
> 
> The allocation granularity should be bytes, in my opinion.  The point
> of the UC allocator is so that we can share a single 64MB UC granule
> across the multiple users (of which there are a grand total of 2, at
> the moment...).
> 
> 	--david
> 

If granularity is bytes, we need to support page-aligned allocations. Fetchop
pages are mmap'ed to user space.

As far as fetchop is concerned, we always allocate 1 page at a time.

-- 
Thanks

Jack Steiner    (651-683-5302)   (vnet 233-5302)      steiner@sgi.com

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christopher Wedgwood]

On Tue, Jul 29, 2003 at 03:47:34PM -0700, Luck, Tony wrote:

> min_state areas for MCA will be smaller than a page ... so *my* use
> of this allocator doesn't require multi-page.

I'm guessing you'll only need 1k or 4k per-cpu right?  What about any
padding here?

On Tue, Jul 29, 2003 at 03:54:27PM -0700, David Mosberger wrote:

> Let's stick with a tried-and-true malloc/free-like interface (nobody
> every gets size arguments to free() right, do they?).

I'd like to claim this is too complex and that page would suffice
except clearly for MINSTATE purposes page granularity is wasteful.
Jack also pointed out for MINSTATE handling on SN2 we would want
local-node pages so perhaps something like:

    u64 ia64_uc_alloc(u64 nbytes, int nodeid, int flags);
    void ia64_uc)free(u64 paddr);

Simply claim '0' is not a useful physical address in this sense and
thus signifies and error?

The 'flags' seems overly complex but would be a mechanism to insist
that memory is allocated from the given node (as opposed to just a
hint for locality).

Comments?



  --cw

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christopher Wedgwood]

On Tue, Jul 29, 2003 at 06:56:15PM -0500, Jack Steiner wrote:

> If granularity is bytes, we need to support page-aligned
> allocations. Fetchop pages are mmap'ed to user space.

We could insist the allocator always return naturally aligned
allocations --- a page-size allocation will be page-aligned.  The only
kicker here is that someone wishing to allocate a large chunk of UC
space might get a failure where they otherwise would not.


  --cw

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[David Mosberger]

>>>>> On Tue, 29 Jul 2003 18:56:15 -0500 (CDT), Jack Steiner <steiner@sgi.com> said:

  Jack> If granularity is bytes, we need to support page-aligned
  Jack> allocations. Fetchop pages are mmap'ed to user space.

I see.  But then EFI-page-aligned memory won't do you no good either
(i.e., you'd need a PAGE_SIZE-based allocator).  Alternatively, just
allocated PAGE_SIZE+1 extra bytes and do the alignment yourself?

	--david

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[David Mosberger]

>>>>> On Tue, 29 Jul 2003 17:00:26 -0700, Christopher Wedgwood <cw@sgi.com> said:

  Christopher> On Tue, Jul 29, 2003 at 03:54:27PM -0700, David Mosberger wrote:

  >> Let's stick with a tried-and-true malloc/free-like interface (nobody
  >> every gets size arguments to free() right, do they?).

  Christopher> I'd like to claim this is too complex

I don't buy this.  Just store a header along with the actual data
(yes, accesses to the header will be slow, but nobody cares).

  Christopher> Jack also pointed out for MINSTATE handling on SN2 we
  Christopher> would want local-node pages so perhaps something like:

  Christopher> u64 ia64_uc_alloc(u64 nbytes, int nodeid, int flags);
  Christopher> void ia64_uc_free(u64 paddr);

  Christopher> Simply claim '0' is not a useful physical address in
  Christopher> this sense and thus signifies and error?

  Christopher> The 'flags' seems overly complex but would be a
  Christopher> mechanism to insist that memory is allocated from the
  Christopher> given node (as opposed to just a hint for locality).

  Christopher> Comments?

The "uc" should be changed to "ucmem" or something like that, to avoid
confusion with allocating uncached address space (what we're
allocating here is normal memory which is mapped uncached).

	--david

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Jack Steiner]

> 
> >>>>> On Tue, 29 Jul 2003 18:56:15 -0500 (CDT), Jack Steiner <steiner@sgi.com> said:
> 
>   Jack> If granularity is bytes, we need to support page-aligned
>   Jack> allocations. Fetchop pages are mmap'ed to user space.
> 
> I see.  But then EFI-page-aligned memory won't do you no good either
> (i.e., you'd need a PAGE_SIZE-based allocator).  Alternatively, just
> allocated PAGE_SIZE+1 extra bytes and do the alignment yourself?
> 
> 	--david

That would work but would waste space (unless we allow "free" to
accept a partial free of a previously allocated chunk. "free" would
have to combine fragments). Fetchop space on our platform is
a limited resource.


-- 
Thanks

Jack Steiner    (651-683-5302)   (vnet 233-5302)      steiner@sgi.com

Re: [PATCH] (2.4.x bk) efi_memmap_walk_uc

[Christoph Hellwig]

On Tue, Jul 29, 2003 at 11:34:07AM -0700, Christopher Wedgwood wrote:
> 	count = (end - start) >> PAGE_SHIFT;
> 	bytes = sizeof(struct node_fetchops) + count/8;
> 	fops = vmalloc(bytes);
> 	memset(fops, 0, bytes);

vmalloc can fail..