[prev in list] [next in list] [prev in thread] [next in thread] 

List:       xen-ppc-devel
Subject:    Re: [XenPPC] [PATCH] [UPDATE] Xencomm optimzation to work for modules
From:       Jerone Young <jyoung5 () us ! ibm ! com>
Date:       2007-01-26 19:09:08
Message-ID: 1169838548.3207.3.camel () thinkpad
[Download RAW message or body]

I'll need to rework the patch. I see there are some inadvertant bugs ..
but somehow it did pass testing. I'll fix these up and redo the patch.
Thanks for the feedback.

On Fri, 2007-01-26 at 14:05 -0500, Jimi Xenidis wrote:
> On Jan 25, 2007, at 8:06 PM, Jerone Young wrote:
> 
> > This patch should address all the issues Jimi has pointed out.
> yes it does... thank you.
> but still has a few issues..
> 
> Throughout the patch:
> 1. xencomm_create_inline() could never fail, xencomm_map() can so you  
> need to check ALL of them.
> 1. _inline() never allocates _map() can so you always have to call  
> xencomm_free()
> 1. Please return a -ERRNO and not -1 all the time.
> 1. you made the descriptor void * but not everywhere.
> 
> other specific issues:
> 
> > @@ -246,9 +244,9 @@ static int xenppc_privcmd_domctl(privcmd
> > 		return -EACCES;
> > 	}
> > -	ret = xencomm_create(&kern_op, sizeof(xen_domctl_t), &op_desc,  
> > GFP_KERNEL);
> > -	if (ret)
> > -		return ret;
> > +	op_desc = xencomm_map(&kern_op, sizeof(xen_domctl_t));
> > +	if (op_desc)
> > +		return -1;
> 
> if (op_desc) cannot be right.
> Can't see how domain creation could have possibly worked, did you  
> test that?
> 
> 
> > diff -r bbf2db4ddf54 drivers/xen/core/xencomm.c
> > --- a/drivers/xen/core/xencomm.c	Tue Dec 19 09:22:37 2006 -0500
> > +++ b/drivers/xen/core/xencomm.c	Wed Jan 26 02:59:31 2028 -0600
> > @@ -82,11 +82,11 @@ static struct xencomm_desc *xencomm_allo
> > void xencomm_free(struct xencomm_desc *desc)
> 
> _map() returns a "void *" so _free() should take one.
> 
> > {
> > -	if (desc)
> > +	if (desc && !((ulong)desc & XENCOMM_INLINE_FLAG))
> > 		free_page((unsigned long)desc);
> > }
> > -int xencomm_create(void *buffer, unsigned long bytes, struct  
> > xencomm_desc **ret, gfp_t gfp_mask)
> > +static int xencomm_create(void *buffer, unsigned long bytes,  
> > struct xencomm_desc **ret, gfp_t gfp_mask)
> > {
> > 	struct xencomm_desc *desc;
> > 	int rc;
> > @@ -119,13 +119,68 @@ int xencomm_create(void *buffer, unsigne
> > 	return 0;
> > }
> > -void *xencomm_create_inline(void *ptr)
> > +static void *xencomm_create_inline(void *ptr)
> > {
> > 	unsigned long paddr;
> > -	BUG_ON(!is_kernel_addr((unsigned long)ptr));
> > +	BUG_ON(!is_phys_contiguous((unsigned long)ptr));
> > 	paddr = (unsigned long)xencomm_pa(ptr);
> > 	BUG_ON(paddr & XENCOMM_INLINE_FLAG);
> > 	return (void *)(paddr | XENCOMM_INLINE_FLAG);
> > }
> > +
> > +/* "mini" routine, for stack-based communications: */
> > +static int xencomm_create_mini(int arealen, void *buffer,
> > +	unsigned long bytes, struct xencomm_desc **ret)
> > +{
> > +	struct xencomm_desc desc;
> 
> You are returning a stack/auto variable here, thats a No No!
> 
> > +	int rc = 0;
> > +
> > +	desc.nr_addrs = XENCOMM_MINI_ADDRS;
> > +
> > +	if (! (rc = xencomm_init(&desc, buffer, bytes)));
> > +		*ret = &desc;
> > +
> > +	return rc;
> > +}
> > +
> > +void *xencomm_map(void *ptr, unsigned long bytes)
> > +{
> > +	int rc;
> > +	struct xencomm_desc *desc;
> > +
> > +	if (is_phys_contiguous((unsigned long)ptr))
> > +		return xencomm_create_inline(ptr);
> > +
> > +	rc = xencomm_create(ptr, bytes, &desc, GFP_KERNEL);
> > +
> > +	if (rc)
> > +		return NULL;
> > +
> > +	return xencomm_pa(desc);
> > +}
> > +
> > +void *__xencomm_map_early(void *ptr, unsigned long bytes,
> > +			struct xencomm_mini *xc_area)
> > +{
> > +	int rc;
> > +	struct xencomm_desc *desc = NULL;
> > +
> > +	if (is_phys_contiguous((unsigned long)ptr))
> > +		return xencomm_create_inline(ptr);
> > +
> > +	rc = xencomm_create_mini(XENCOMM_MINI_AREA,ptr, bytes,
> 
> whitespace
> 
> > +				&desc);
> > +
> > +	if (rc)
> > +		return NULL;
> > +
> > +	return (void*)__pa(desc);
> > +}
> > +
> > +/* check if is physically contiguous memory */
> > +int is_phys_contiguous(unsigned long addr)
> 
> Can we please make this static now!
> 
> > +{
> > +	return (addr < VMALLOC_START) || (addr >= VMALLOC_END);
> > +}
> > diff -r bbf2db4ddf54 include/xen/xencomm.h
> > --- a/include/xen/xencomm.h	Tue Dec 19 09:22:37 2006 -0500
> > +++ b/include/xen/xencomm.h	Wed Jan 26 02:17:31 2028 -0600
> > @@ -16,6 +16,7 @@
> >   * Copyright (C) IBM Corp. 2006
> >   *
> >   * Authors: Hollis Blanchard <hollisb@us.ibm.com>
> > + *          Jerone Young <jyoung5@us.ibm.com>
> >   */
> > #ifndef _LINUX_XENCOMM_H_
> > @@ -23,10 +24,23 @@
> > #include <xen/interface/xencomm.h>
> > -extern int xencomm_create(void *buffer, unsigned long bytes,
> > -			  struct xencomm_desc **desc, gfp_t type);
> > +#define XENCOMM_MINI_ADDRS 3
> > +struct xencomm_mini {
> > +	struct xencomm_desc _desc;
> > +	uint64_t address[XENCOMM_MINI_ADDRS];
> > +};
> > +#define XENCOMM_MINI_AREA (sizeof(struct xencomm_mini) * 2)
> 
> This is no longer used, right?
> 
> > +
> > extern void xencomm_free(struct xencomm_desc *desc);
> > -extern void *xencomm_create_inline(void *ptr);
> > +extern void *xencomm_map(void *ptr, unsigned long bytes);
> > +extern void *__xencomm_map_early(void *ptr, unsigned long bytes,
> > +				struct xencomm_mini *xc_area);
> > +extern int is_phys_contiguous(unsigned long addr);
> begone
> 
> > +
> > +#define xencomm_map_early(ptr, bytes) \
> > +	({struct xencomm_mini xc_area\
> > +		__attribute__((__aligned__(sizeof(struct xencomm_mini))));\
> > +		__xencomm_map_early(ptr, bytes, &xc_area);})
> > /* provided by architecture code: */
> > extern unsigned long xencomm_vtop(unsigned long vaddr);
> 


_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@lists.xensource.com
http://lists.xensource.com/xen-ppc-devel
[prev in list] [next in list] [prev in thread] [next in thread]