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

[annie li <annie.li@oracle.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7332 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 8459 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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