From mboxrd@z Thu Jan 1 00:00:00 1970 From: annie li Subject: Re: [PATCH 1/3] Introducing grant table V2 stucture Date: Thu, 10 Nov 2011 00:19:25 +0800 Message-ID: <4EBAA80D.5050300@oracle.com> References: <4EBA35D3.3020506@oracle.com> <1320826490-29362-1-git-send-email-annie.li@oracle.com> <1320845283.955.131.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1039684802==" Return-path: In-Reply-To: <1320845283.955.131.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Campbell Cc: "jeremy@goop.org" , "xen-devel@lists.xensource.com" , "konrad.wilk@oracle.com" , "kurt.hackel@oracle.com" , "linux-kernel@vger.kernel.org" , Paul Durrant List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============1039684802== Content-Type: multipart/alternative; boundary="------------000104060803070302010500" This is a multi-part message in MIME format. --------------000104060803070302010500 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 2011-11-9 21:28, Ian Campbell wrote: > On Wed, 2011-11-09 at 08:14 +0000, annie.li@oracle.com wrote: > >> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c >> index 4f44b34..0d481a9 100644 >> --- a/drivers/xen/grant-table.c >> +++ b/drivers/xen/grant-table.c >> @@ -53,7 +53,7 @@ >> /* External tools reserve first few grant table entries. */ >> #define NR_RESERVED_ENTRIES 8 >> #define GNTTAB_LIST_END 0xffffffff >> -#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry)) >> +#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry_v1)) >> > > Does this need to become GREFS_V1_PER... or something? > This is common definition for both V1 and V2, and is used by both v1 and v2 implementation. If it is changed to GREFS_V1_PER... or something with v1 tag in this patch, it is necessary to change it back in following "grant table v2 implementation" patch again. So I just keep it unchanged here. > >> static grant_ref_t **gnttab_list; >> static unsigned int nr_grant_frames; >> @@ -64,7 +64,72 @@ static DEFINE_SPINLOCK(gnttab_list_lock); >> unsigned long xen_hvm_resume_frames; >> EXPORT_SYMBOL_GPL(xen_hvm_resume_frames); >> >> -static struct grant_entry *shared; >> +static union { >> + struct grant_entry_v1 *v1; >> + void *ring_addr; >> +} shared; >> + >> +/* >> + * This function is null for grant table v1, adding it here in order to keep >> + * consistent with *_v2 interface. >> + */ >> +static int gnttab_map_status_v1(unsigned int nr_gframes); >> +/* >> + * This function is null for grant table v1, adding it here in order to keep >> + * consistent with *_v2 interface. >> + */ >> +static void gnttab_unmap_status_v1(void); >> > > If you reorder the declarations of gnttab_request_version and > gnttab_(un)map_status_v1 below then you can avoid these forward > declarations. > Yes, you are right, thanks. > Also I don't think the comment really adds much once you have the empty > declaration underneath (like you do below). A simple "/* Nothing to do > for v1 */" in the implementation would be sufficient. > I agree. > >> + >> +/*This is a structure of function pointers for grant table v1*/ >> > > and eventually v2, right? Actually I think the v1 is unnecessary. e.g.: > /*This is a structure of function pointers for grant table*/ > > Thanks, will change it. >> +static struct { >> + /* >> + * Mapping a list of frames for storing grant entry status, this >> + * mechanism can allow better synchronization using barriers. Input >> + * parameter is frame number, returning GNTST_okay means success and >> + * negative value means failure. >> + */ >> + int (*_gnttab_map_status)(unsigned int); >> > > I think you can drop the "_gnttab_" prefix from all of these, it'll be > clear from the gnttab->foo what the namespace is. > Ok, no problem. > [...] > >> +} gnttab_interface; >> + >> +static int grant_table_version; >> >> static struct gnttab_free_callback *gnttab_free_callback_list; >> >> > > >> @@ -155,12 +229,10 @@ static void update_grant_entry(grant_ref_t ref, domid_t domid, >> * 3. Write memory barrier (WMB). >> * 4. Write ent->flags, inc. valid type. >> */ >> - shared[ref].frame = frame; >> - shared[ref].domid = domid; >> - wmb(); >> - shared[ref].flags = flags; >> + gnttab_interface._update_grant_entry(ref, domid, frame, flags); >> } >> >> + >> > > Please avoid unrelated whitespace changes. > Oh, it was ignored. Thanks, i will delete it. > >> /* >> * Public grant-issuing interface functions >> */ >> @@ -187,31 +259,40 @@ int gnttab_grant_foreign_access(domid_t domid, unsigned long frame, >> } >> EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access); >> >> -int gnttab_query_foreign_access(grant_ref_t ref) >> +int gnttab_query_foreign_access_v1(grant_ref_t ref) >> > > This and all the other similar functions can now be made static. > Ok, got it, thanks. Thanks Annie > Ian. > > --------------000104060803070302010500 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

On 2011-11-9 21:28, Ian Campbell wrote: 
On Wed, 2011-11-09 at 08:14 +0000, annie.li@oracle.com wrote:
  
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 4f44b34..0d481a9 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -53,7 +53,7 @@
 /* External tools reserve first few grant table entries. */
 #define NR_RESERVED_ENTRIES 8
 #define GNTTAB_LIST_END 0xffffffff
-#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry))
+#define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry_v1))
    

Does this need to become GREFS_V1_PER... or something?
This is common definition for both V1 and V2, and is used by both v1 and v2 implementation. If it is changed to GREFS_V1_PER... or something with v1 tag in this patch, it is necessary to change it back in following "grant table v2 implementation" patch again. So I just keep it unchanged here. 
  
 static grant_ref_t **gnttab_list;
 static unsigned int nr_grant_frames;
@@ -64,7 +64,72 @@ static DEFINE_SPINLOCK(gnttab_list_lock);
 unsigned long xen_hvm_resume_frames;
 EXPORT_SYMBOL_GPL(xen_hvm_resume_frames);

-static struct grant_entry *shared;
+static union {
+       struct grant_entry_v1 *v1;
+       void *ring_addr;
+} shared;
+
+/*
+ * This function is null for grant table v1, adding it here in order to keep
+ * consistent with *_v2 interface.
+ */
+static int gnttab_map_status_v1(unsigned int nr_gframes);
+/*
+ * This function is null for grant table v1, adding it here in order to keep
+ * consistent with *_v2 interface.
+ */
+static void gnttab_unmap_status_v1(void);
    

If you reorder the declarations of gnttab_request_version and
gnttab_(un)map_status_v1 below then you can avoid these forward
declarations.
Yes, you are right, thanks.
Also I don't think the comment really adds much once you have the empty
declaration underneath (like you do below). A simple "/* Nothing to do
for v1 */" in the implementation would be sufficient.
I agree.
  
+
+/*This is a structure of function pointers for grant table v1*/
    

and eventually v2, right? Actually I think the v1 is unnecessary. e.g.:
	/*This is a structure of function pointers for grant table*/
Thanks, will change it.

  

    
+static struct {
+       /*
+        * Mapping a list of frames for storing grant entry status, this
+        * mechanism can allow better synchronization using barriers. Input
+        * parameter is frame number, returning GNTST_okay means success and
+        * negative value means failure.
+        */
+       int (*_gnttab_map_status)(unsigned int);
    

  

  

I think you can drop the "_gnttab_" prefix from all of these, it'll be
clear from the gnttab->foo what the namespace is.
  



Ok, no problem.



  
[...]
  

  

    
+} gnttab_interface;
+
+static int grant_table_version;

 static struct gnttab_free_callback *gnttab_free_callback_list;

    

  

  

  

  

    
@@ -155,12 +229,10 @@ static void update_grant_entry(grant_ref_t ref, domid_t domid,
         *  3. Write memory barrier (WMB).
         *  4. Write ent->flags, inc. valid type.
         */
-       shared[ref].frame = frame;
-       shared[ref].domid = domid;
-       wmb();
-       shared[ref].flags = flags;
+       gnttab_interface._update_grant_entry(ref, domid, frame, flags);
 }

+
    

  

  

Please avoid unrelated whitespace changes.
  



Oh, it was ignored. Thanks, i will delete it.



  
  

  

    
 /*
  * Public grant-issuing interface functions
  */
@@ -187,31 +259,40 @@ int gnttab_grant_foreign_access(domid_t domid, unsigned long frame,
 }
 EXPORT_SYMBOL_GPL(gnttab_grant_foreign_access);

-int gnttab_query_foreign_access(grant_ref_t ref)
+int gnttab_query_foreign_access_v1(grant_ref_t ref)
    

  

  

This and all the other similar functions can now be made static.
  



Ok, got it, thanks.



Thanks

Annie



  
Ian.

  






--------------000104060803070302010500--


--===============1039684802==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

--===============1039684802==--