Re: [Xen-devel] [PATCH v3 04/15] argo: init, destroy and soft-reset, with enable command line opt

["Jan Beulich" <JBeulich@xxxxxxxx>]

 >>> On 10.01.19 at 11:19, <royger@xxxxxxxxxxx> wrote:
> aOn Mon, Jan 7, 2019 at 8:44 AM Christopher Clark 
> <christopher.w.clark@xxxxxxxxx> wrote:
>>
>> +/* Xen command line option to enable argo */
>> +static bool __read_mostly opt_argo_enabled;
>> +boolean_param("argo", opt_argo_enabled);
> 
> I would drop the opt_* prefix, new options added recently don't
> include the prefix already.

Would you mind pointing out examples? Especially for boolean ones I
think we've tried to consistently name them opt_*. But in the case
here (it being static) I'm not overly fussed.

>> +
>> +typedef struct argo_ring_id
>> +{
>> +    uint32_t port;
>> +    domid_t partner_id;
>> +    domid_t domain_id;
>> +} argo_ring_id;
>> +
>> +/* Data about a domain's own ring that it has registered */
>> +struct argo_ring_info
>> +{
>> +    /* next node in the hash, protected by L2 */
>> +    struct hlist_node node;
>> +    /* this ring's id, protected by L2 */
>> +    struct argo_ring_id id;
>> +    /* L3 */
>> +    spinlock_t lock;
>> +    /* length of the ring, protected by L3 */
>> +    uint32_t len;
>> +    /* number of pages in the ring, protected by L3 */
>> +    uint32_t npage;
> 
> Can you infer number of pages form the length of the ring, or the
> other way around?
> 
> I'm not sure why both need to be stored here.
> 
>> +    /* number of pages translated into mfns, protected by L3 */
>> +    uint32_t nmfns;
>> +    /* cached tx pointer location, protected by L3 */
>> +    uint32_t tx_ptr;
> 
> All this fields are not part of any public structure, so I wonder if
> it would be better to simply use unsigned int for those, or size_t.

Yes indeed - there's way too much use of fixed width types here.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel