On Jan 18, 2007, at 1:55 PM, Hollis Blanchard wrote:
On Thu, 2007-01-18 at 12:17 -0500, Jimi Xenidis wrote:
I agree with most of Hollis's comments, but have some of my own.
First, I do not think that the implementation of is_phys_contiguous()
answers the question in its name and IMNSHO is bogus. Perhaps
something like:
mm/sparse.c vaddr_in_vmalloc_area 232 static int
vaddr_in_vmalloc_area(void *addr)
And use if (!vaddr_in_vmalloc_area)
The name was my suggestion. It should be commented, but think about
it:
we don't care if something is vmalloc or not. We care if it's
physically
contiguous or not, so I strongly believe that should be the name of
the
test.
I'm not big on functions that do not implement what the name says it
does.
However, the worst that can happen is a false-negative, (unless it is
an ioremap() address which would be other problems).
Hey, wouldn't virt_addr_valid() do?
More importantly... (and this needs to be addressed before the patch
is accepted)
I like the "map" name change, but not sure about "early".
oops this last line slipped by.. I wrote the email over multiple
sittings.
What is your request? Just renaming it?
My point is the "early" is not the only reason mini was used.
There are 2 reasons why we had mini/inline/early:
1) because the allocator was not ready, I think this applies to a
small number of hcalls
2) we cannot "sleep" (in_interrupt()) and the allocator sleeps,
Mainly evtchn related and console.
So early covers (1) but (2) will be problematic, I noted the ones
below that may reflect (2), I for one, have not been diligent in
commenting why mini/inline is actually used and I think we need to do
so.
What, add a comment describing a code path that may change in the
future? I would object to that.
Was that sarcasm?
So giving this some more thought, and jerking you around even more
(sorry), aside from using inline to optimize this:
- We can detect (1) with slab_is_available() and use mini
- We can detect (2) with in_interrupt() and try GFP_ATOMIC then
mini.
Not a request, but something to think about.
What *is* your request then, the one that needs to be addressed?
My request is that ,at a minimum, everything that was inline/mini
must be _early because we cannot alloc anything while in_interrupt().
Ultimately, we call the right allocator at the right time.
And since we have interfaces to detect both early and in_interrupt we
can probably bring this down to a single inteligent xencomm_map() call.
Mini, has always bugged me, it seems to me that this could be
satisfied by a per_cpu static, or preallocated buffer to for the xen
descriptor. This may not be viable because it assumes no interrupts,
and though for asynchronous interrupts this may be a safe assumption,
if one were to suffer a series of page faults (from a wild pointer in
this path) we would have a silent hang, which is never good.
So mini has always bugged you, but you agree there's no better way
to do
it and are just thinking out loud?
yes.
-JX
_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel
|
Home