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

From: "Jan Beulich" <JBeulich@suse.com>
To: royger@FreeBSD.org, Christopher Clark <christopher.w.clark@gmail.com>
Cc: Tim Deegan <tim@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	ross.philipson@gmail.com, Jason Andryuk <jandryuk@gmail.com>,
	Daniel Smith <dpsmith@apertussolutions.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>,
	Rich Persaud <persaur@gmail.com>,
	James McKenzie <james@bromium.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Paul Durrant <paul.durrant@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	eric chanudet <eric.chanudet@gmail.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v3 04/15] argo: init, destroy and soft-reset, with enable command line opt
Date: Thu, 10 Jan 2019 04:52:09 -0700	[thread overview]
Message-ID: <5C3731E9020000780020C2AD@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <CAPLaKK6z4eNtJx+X3RdTcO8tKb+PxaeBwA_N3wDmbXc7Qss58A@mail.gmail.com>

 >>> On 10.01.19 at 11:19, <royger@freebsd.org> wrote:
> aOn Mon, Jan 7, 2019 at 8:44 AM Christopher Clark <christopher.w.clark@gmail.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel