Re: [XenPPC] [PATCH] [RFC] Xencomm patch to fix modules

[Jimi Xenidis <jimix@xxxxxxxxxxxxxx>]

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