[Xen-devel] Re: [PATCH 1/3] xen/granttable: Introducing grant table V2 stucture

[annie li <annie.li@xxxxxxxxxx>]

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

    

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