From mboxrd@z Thu Jan 1 00:00:00 1970 From: annie li Subject: Re: [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture Date: Fri, 18 Nov 2011 00:06:49 +0800 Message-ID: <4EC53119.7060307@oracle.com> References: <4EC3B62F.6080702@oracle.com> <1321451304-13559-1-git-send-email-annie.li@oracle.com> <1321526148.3664.263.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1020048056==" Return-path: In-Reply-To: <1321526148.3664.263.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. --===============1020048056== Content-Type: multipart/alternative; boundary="------------090106010607000406010701" This is a multi-part message in MIME format. --------------090106010607000406010701 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Thanks for your review, Ian. See following, >> - } while ((nflags = sync_cmpxchg(&shared[ref].flags, flags, 0)) != flags); >> + } while ((nflags = sync_cmpxchg(&gnttab_shared.v1[ref].flags, flags, 0)) >> + != flags); >> > > I think this is one of those cases where strictly adhering to an > 80-column rule hurts the readability of the code. > > If you had left the static global as "shared" rather than > "gnttab_shared" you wouldn't have this issue. If you want a more > descriptive name why not just call it "gnttab"? > > Actually, whether the name is "gnttab_shared" or "shared" or "gnttab", the code line still breaks the 80-column rule. >> return 1; >> } >> + >> +int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly) >> +{ >> + return gnttab_interface.end_foreign_access_ref(ref, readonly); >> +} >> EXPORT_SYMBOL_GPL(gnttab_end_foreign_access_ref); >> >> void gnttab_end_foreign_access(grant_ref_t ref, int readonly, >> @@ -246,37 +309,45 @@ EXPORT_SYMBOL_GPL(gnttab_grant_foreign_transfer); >> void gnttab_grant_foreign_transfer_ref(grant_ref_t ref, domid_t domid, >> unsigned long pfn) >> { >> - update_grant_entry(ref, domid, pfn, GTF_accept_transfer); >> + gnttab_interface.update_entry(ref, domid, pfn, GTF_accept_transfer); >> } >> EXPORT_SYMBOL_GPL(gnttab_grant_foreign_transfer_ref); >> >> -unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref) >> +static unsigned long gnttab_end_foreign_transfer_ref_v1(grant_ref_t ref) >> { >> unsigned long frame; >> u16 flags; >> + u16 *pflags; >> + >> + pflags = &gnttab_shared.v1[ref].flags; >> > > It would be nice if these refactoring bits could be separated out from > the more mechanical renaming and abstracting to fn pointer aspects of > the patch. > I am not so sure about your meaning, do you mean change gnttab_shared back to shared? >> /* >> * If a transfer is not even yet started, try to reclaim the grant >> * reference and return failure (== 0). >> */ >> - while (!((flags = shared[ref].flags) & GTF_transfer_committed)) { >> - if (sync_cmpxchg(&shared[ref].flags, flags, 0) == flags) >> + while (!((flags = *pflags) & GTF_transfer_committed)) { >> + if (sync_cmpxchg(pflags, flags, 0) == flags) >> return 0; >> cpu_relax(); >> } >> >> /* If a transfer is in progress then wait until it is completed. */ >> while (!(flags & GTF_transfer_completed)) { >> - flags = shared[ref].flags; >> + flags = *pflags; >> cpu_relax(); >> } >> >> rmb(); /* Read the frame number /after/ reading completion status. */ >> - frame = shared[ref].frame; >> + frame = gnttab_shared.v1[ref].frame; >> BUG_ON(frame == 0); >> >> return frame; >> } >> + >> +unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref) >> +{ >> + return gnttab_interface.end_foreign_transfer_ref(ref); >> +} >> EXPORT_SYMBOL_GPL(gnttab_end_foreign_transfer_ref); >> >> unsigned long gnttab_end_foreign_transfer(grant_ref_t ref) >> @@ -520,6 +591,23 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops, >> } >> EXPORT_SYMBOL_GPL(gnttab_unmap_refs); >> >> +static int gnttab_map_frames_v1(unsigned long *frames, unsigned int nr_gframes) >> +{ >> + int rc; >> + >> + rc = arch_gnttab_map_shared(frames, nr_gframes, >> + gnttab_max_grant_frames(), >> + &gnttab_shared.addr); >> + BUG_ON(rc); >> + >> + return 0; >> +} >> + >> +static void gnttab_unmap_frames_v1(void) >> +{ >> + arch_gnttab_unmap_shared(gnttab_shared.addr, nr_grant_frames); >> +} >> + >> static int gnttab_map(unsigned int start_idx, unsigned int end_idx) >> { >> struct gnttab_setup_table setup; >> @@ -567,19 +655,33 @@ static int gnttab_map(unsigned int start_idx, unsigned int end_idx) >> >> BUG_ON(rc || setup.status); >> >> - rc = arch_gnttab_map_shared(frames, nr_gframes, gnttab_max_grant_frames(), >> - &shared); >> - BUG_ON(rc); >> + rc = gnttab_interface.map_frames(frames, nr_gframes); >> > > Nothing checks rc here now? > > In fact the gnttab_map_frames_v1 function has its own BUG_ON(rc) and > always returns 0 if it returns at all so perhaps that hook should be > returning void? > Yes, it should be that if there is only v1 function existing. However, I added returns 0 here in order to keep consistence with v2 function of next patch. The function pointer type is: int (*map_frames)(....), and v2 function returning value is meaningful. The returning value directly decides returning value of gnttab_map. See following code in function gnttab_map of v2 patch: if (rc < 0) return rc; return 0; If gnttab_map_frames_v1 returns void here, it is necessary to change it back(including "void (*map_frames)" --> "int (*map_frames)") in next v2 implementation patch. So I only added return 0 here. > >> kfree(frames); >> >> return 0; >> } >> >> +static void gnttab_request_version(void) >> +{ >> + grant_table_version = 1; >> + gnttab_interface.map_frames = gnttab_map_frames_v1; >> + gnttab_interface.unmap_frames = gnttab_unmap_frames_v1; >> + gnttab_interface.update_entry = update_grant_entry_v1; >> + gnttab_interface.end_foreign_access_ref = >> + gnttab_end_foreign_access_ref_v1; >> + gnttab_interface.end_foreign_transfer_ref = >> + gnttab_end_foreign_transfer_ref_v1; >> + gnttab_interface.query_foreign_access = gnttab_query_foreign_access_v1; >> > > The more normal way to do this would be to make gnttab_interface a > pointer, define gnttab_v1_ops and do: > gnttab_interface = &gnttab_v1_ops; > or if the pointer overhead is significant remove that and just do a > struct assignment: > gnttab_interface = gnttab_v1_ops; > > If using this way, we need two more public structures(gnttab_v1_ops and gnttab_v2_ops), and two more functions to initialize those two structures and then initialize the pointer gnttab_interface. It is more complicated, am i missing something? ..... >> +/* >> * Bitfield values for update_pin_status.flags. >> */ >> /* Map the grant entry for access by I/O devices. */ >> diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h >> index 6a6e914..710afe0 100644 >> --- a/include/xen/interface/xen.h >> +++ b/include/xen/interface/xen.h >> @@ -523,6 +523,8 @@ struct tmem_op { >> } u; >> }; >> >> +DEFINE_GUEST_HANDLE(uint64_t); >> > > The kernel uses uN style types rather than the uintN_t style ones, > although include/xen/interface/grant_table.h seems not to adhere to that > at the moment. It might be worth cleaning that up as you go passed. > Thanks, I'd like to change it. Thanks Annie > >> + >> #else /* __ASSEMBLY__ */ >> >> /* In assembly code we cannot use C numeric constant suffixes. */ >> -- >> 1.7.6.4 >> >> > > > --------------090106010607000406010701 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks for your review, Ian.
See following,
-       } while ((nflags = sync_cmpxchg(&shared[ref].flags, flags, 0)) != flags);
+       } while ((nflags = sync_cmpxchg(&gnttab_shared.v1[ref].flags, flags, 0))
+                       != flags);
    

I think this is one of those cases where strictly adhering to an
80-column rule hurts the readability of the code.

If you had left the static global as "shared" rather than
"gnttab_shared" you wouldn't have this issue. If you want a more
descriptive name why not just call it "gnttab"?
Actually, whether the name is "gnttab_shared" or "shared" or "gnttab", the code line still breaks the 80-column rule.

  

    
        return 1;
 }
+
+int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
+{
+       return gnttab_interface.end_foreign_access_ref(ref, readonly);
+}
 EXPORT_SYMBOL_GPL(gnttab_end_foreign_access_ref);

 void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
@@ -246,37 +309,45 @@ EXPORT_SYMBOL_GPL(gnttab_grant_foreign_transfer);
 void gnttab_grant_foreign_transfer_ref(grant_ref_t ref, domid_t domid,
                                       unsigned long pfn)
 {
-       update_grant_entry(ref, domid, pfn, GTF_accept_transfer);
+       gnttab_interface.update_entry(ref, domid, pfn, GTF_accept_transfer);
 }
 EXPORT_SYMBOL_GPL(gnttab_grant_foreign_transfer_ref);

-unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref)
+static unsigned long gnttab_end_foreign_transfer_ref_v1(grant_ref_t ref)
 {
        unsigned long frame;
        u16           flags;
+       u16          *pflags;
+
+       pflags = &gnttab_shared.v1[ref].flags;
    

  

  

It would be nice if these refactoring bits could be separated out from
the more mechanical renaming and abstracting to fn pointer aspects of
the patch.
  



I am not so sure about your meaning, do you mean change gnttab_shared
back to shared?



  

    
        /*
         * If a transfer is not even yet started, try to reclaim the grant
         * reference and return failure (== 0).
         */
-       while (!((flags = shared[ref].flags) & GTF_transfer_committed)) {
-               if (sync_cmpxchg(&shared[ref].flags, flags, 0) == flags)
+       while (!((flags = *pflags) & GTF_transfer_committed)) {
+               if (sync_cmpxchg(pflags, flags, 0) == flags)
                        return 0;
                cpu_relax();
        }

        /* If a transfer is in progress then wait until it is completed. */
        while (!(flags & GTF_transfer_completed)) {
-               flags = shared[ref].flags;
+               flags = *pflags;
                cpu_relax();
        }

        rmb();  /* Read the frame number /after/ reading completion status. */
-       frame = shared[ref].frame;
+       frame = gnttab_shared.v1[ref].frame;
        BUG_ON(frame == 0);

        return frame;
 }
+
+unsigned long gnttab_end_foreign_transfer_ref(grant_ref_t ref)
+{
+       return gnttab_interface.end_foreign_transfer_ref(ref);
+}
 EXPORT_SYMBOL_GPL(gnttab_end_foreign_transfer_ref);

 unsigned long gnttab_end_foreign_transfer(grant_ref_t ref)
@@ -520,6 +591,23 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
 }
 EXPORT_SYMBOL_GPL(gnttab_unmap_refs);

+static int gnttab_map_frames_v1(unsigned long *frames, unsigned int nr_gframes)
+{
+       int rc;
+
+       rc = arch_gnttab_map_shared(frames, nr_gframes,
+                                   gnttab_max_grant_frames(),
+                                   &gnttab_shared.addr);
+       BUG_ON(rc);
+
+       return 0;
+}
+
+static void gnttab_unmap_frames_v1(void)
+{
+       arch_gnttab_unmap_shared(gnttab_shared.addr, nr_grant_frames);
+}
+
 static int gnttab_map(unsigned int start_idx, unsigned int end_idx)
 {
        struct gnttab_setup_table setup;
@@ -567,19 +655,33 @@ static int gnttab_map(unsigned int start_idx, unsigned int end_idx)

        BUG_ON(rc || setup.status);

-       rc = arch_gnttab_map_shared(frames, nr_gframes, gnttab_max_grant_frames(),
-                                   &shared);
-       BUG_ON(rc);
+       rc = gnttab_interface.map_frames(frames, nr_gframes);
    

  

  

Nothing checks rc here now?

In fact the gnttab_map_frames_v1 function has its own BUG_ON(rc) and
always returns 0 if it returns at all so perhaps that hook should be
returning void?
  



Yes, it should be that if there is only v1 function existing.

However, I added returns 0 here in order to keep consistence with v2
function of next patch. The function pointer type is: int
(*map_frames)(....), and v2 function returning value is meaningful. The
returning value directly decides returning value of gnttab_map. See
following code in function gnttab_map of v2 patch:



        if (rc < 0)

                return rc;

        return 0;



If gnttab_map_frames_v1
returns void here, it is necessary to change it back(including "void
(*map_frames)"  --> "int (*map_frames)") in next v2 implementation
patch. So I only added return 0 here.



  
  

  

    
        kfree(frames);

        return 0;
 }

+static void gnttab_request_version(void)
+{
+       grant_table_version = 1;
+       gnttab_interface.map_frames = gnttab_map_frames_v1;
+       gnttab_interface.unmap_frames = gnttab_unmap_frames_v1;
+       gnttab_interface.update_entry = update_grant_entry_v1;
+       gnttab_interface.end_foreign_access_ref =
+                                       gnttab_end_foreign_access_ref_v1;
+       gnttab_interface.end_foreign_transfer_ref =
+                                       gnttab_end_foreign_transfer_ref_v1;
+       gnttab_interface.query_foreign_access = gnttab_query_foreign_access_v1;
    

  

  

The more normal way to do this would be to make gnttab_interface a
pointer, define gnttab_v1_ops and do:
	gnttab_interface = &gnttab_v1_ops;
or if the pointer overhead is significant remove that and just do a
struct assignment:
	gnttab_interface = gnttab_v1_ops;

  



If using this way, we need two more public structures(gnttab_v1_ops and
gnttab_v2_ops), and two more functions to initialize those two
structures and then initialize the pointer gnttab_interface. It is more
complicated, am i missing something?



.....



  

    
+/*
  * Bitfield values for update_pin_status.flags.
  */
  /* Map the grant entry for access by I/O devices. */
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 6a6e914..710afe0 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -523,6 +523,8 @@ struct tmem_op {
        } u;
 };

+DEFINE_GUEST_HANDLE(uint64_t);
    

  

  

The kernel uses uN style types rather than the uintN_t style ones,
although include/xen/interface/grant_table.h seems not to adhere to that
at the moment. It might be worth cleaning that up as you go passed.
  



Thanks, I'd like to change it.



Thanks

Annie



  
  

  

    
+
 #else /* __ASSEMBLY__ */

 /* In assembly code we cannot use C numeric constant suffixes. */
--
1.7.6.4

    

  

  


  






--------------090106010607000406010701--


--===============1020048056==
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

--===============1020048056==--