Re: [Xen-devel] [PATCH v5 2/8] xen: clean up grant_table.h

[Juergen Gross <jgross@xxxxxxxx>]

On 08/09/17 17:00, Jan Beulich wrote:
>>>> On 08.09.17 at 08:56, <jgross@xxxxxxxx> wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -40,6 +40,44 @@
>>  #include <xsm/xsm.h>
>>  #include <asm/flushtlb.h>
>>  
>> +/* Per-domain grant information. */
>> +struct grant_table {
>> +    /*
>> +     * Lock protecting updates to grant table state (version, active
>> +     * entry list, etc.)
>> +     */
>> +    percpu_rwlock_t       lock;
>> +    /* Table size. Number of frames shared with guest */
>> +    unsigned int          nr_grant_frames;
>> +    /* Shared grant table (see include/public/grant_table.h). */
>> +    union {
>> +        void **shared_raw;
>> +        struct grant_entry_v1 **shared_v1;
>> +        union grant_entry_v2 **shared_v2;
>> +    };
>> +    /* Number of grant status frames shared with guest (for version 2) */
>> +    unsigned int          nr_status_frames;
>> +    /* State grant table (see include/public/grant_table.h). */
>> +    grant_status_t       **status;
>> +    /* Active grant table. */
>> +    struct active_grant_entry **active;
>> +    /* Mapping tracking table per vcpu. */
>> +    struct grant_mapping **maptrack;
>> +    unsigned int          maptrack_limit;
>> +    /* Lock protecting the maptrack limit */
>> +    spinlock_t            maptrack_lock;
>> +    /*
>> +     * The defined versions are 1 and 2.  Set to 0 if we don't know
>> +     * what version to use yet.
>> +     */
>> +    unsigned              gt_version;
> 
> unsigned int please, for consistency with other code (elsewhere and
> right above).

Yes.

> 
>> +};
> 
> While you're moving it here I think a little bit of re-arrangement
> wouldn't hurt. At the very least nr_status_frames could be put
> next to nr_grant_frames, so there won't be two 4-byte holes. Or
> if there isn't one after nr_grant_frames (i.e. if my counting of
> percpu_rwlock_t members was wrong), gt_version could also be
> moved next to those other two.

I think I'll reorder the structure completely by putting all pointers
at the end and locks and ints at the beginning. That should minimize
the holes.

>> +static inline unsigned int grant_to_status_frames(unsigned int grant_frames)
>> +{
>> +    return (grant_frames * GRANT_PER_PAGE + GRANT_STATUS_PER_PAGE - 1) /
>> +        GRANT_STATUS_PER_PAGE;
> 
> Would be nice if indentation was corrected here at the same time.

DIV_ROUND_UP() seems to be the better choice.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel