(2.4.x bk) efi_memmap_walk_uc

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);

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

As do I.

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?

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

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

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

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;
}

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

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

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.

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

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).

Chris> if (md->attribute = EFI_MEMORY_UC)

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

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.

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.

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

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

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

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).

-Tony

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??

Thanks

Jack Steiner    (651-683-5302)   (vnet 233-5302)      [email protected]

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.

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.

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

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!

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 :-)

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

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

True.

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 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.

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

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.

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

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

It sounds like it.

--cw

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

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

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

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)      [email protected]

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 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

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)      [email protected]

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

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

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

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

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

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)      [email protected]

vmalloc can fail..