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.
If in the future vmalloc space becomes physically contiguous, or some
other region of memory becomes physically discontiguous, we will need to
change the implementation of the function, but the name remains the
same.
> More importantly... (and this needs to be addressed before the patch
> is accepted)
> I like the "map" name change, but not sure about "early".
What is your request? Just renaming it?
> 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.
> 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?
> 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?
> See the comment below that should remove this function entirely,
> Hollis please ACK.
> > +/* "mini" routines, for stack-based communications: */
> > +static void *xencomm_alloc_mini(void *area, int arealen)
> > +{
> > + unsigned long base = (unsigned long)area;
> > + unsigned int left_in_page;
> > +
> > + left_in_page = PAGE_SIZE - base % PAGE_SIZE;
> > +
> > + /* we probably fit right at the front of area */
> > + if (left_in_page >= sizeof(struct xencomm_mini)) {
> > + return area;
> > + }
> > +
> > + /* if not, see if area is big enough to advance to the next page */
> > + if ((arealen - left_in_page) >= sizeof(struct xencomm_mini))
> > + return (void *)(base + left_in_page);
> > +
> > + /* area was too small */
> > + return NULL;
> > +}
...
>
> Since we are completely automating MINI we can simplify (remove) all
> the alignment logic by asking gcc for some help.
>
> > +#define xencomm_map_early(ptr, bytes) \
> > + ({char xc_area[XENCOMM_MINI_AREA];\
> ({char xc_area[XENCOMM_MINI_AREA] \
> __attribute__((__aligned__(sizeof(struct xencomm_mini))));\
>
> In fact it can now be a the struct instead of a char.
Agreed.
--
Hollis Blanchard
IBM Linux Technology Center
_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel
|
Home