Re: [PATCH v3 3/3] docs/doxygen: doxygen documentation for grant_table.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2021 16:59, Luca Fancellu wrote:
>> On 27 Apr 2021, at 14:57, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 26.04.2021 17:37, Luca Fancellu wrote:
>>> @@ -66,6 +67,7 @@
>>>  *     compiler barrier will still be required.
>>>  *
>>>  * Introducing a valid entry into the grant table:
>>> + * @code
>>>  *  1. Write ent->domid.
>>>  *  2. Write ent->frame:
>>>  *      GTF_permit_access:   Frame to which access is permitted.
>>> @@ -73,20 +75,25 @@
>>>  *                           frame, or zero if none.
>>>  *  3. Write memory barrier (WMB).
>>>  *  4. Write ent->flags, inc. valid type.
>>> + * @endcode
>>>  *
>>>  * Invalidating an unused GTF_permit_access entry:
>>> + * @code
>>>  *  1. flags = ent->flags.
>>>  *  2. Observe that !(flags & (GTF_reading|GTF_writing)).
>>>  *  3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0).
>>>  *  NB. No need for WMB as reuse of entry is control-dependent on success of
>>>  *      step 3, and all architectures guarantee ordering of ctrl-dep writes.
>>> + * @endcode
>>>  *
>>>  * Invalidating an in-use GTF_permit_access entry:
>>> + *
>>>  *  This cannot be done directly. Request assistance from the domain 
>>> controller
>>>  *  which can set a timeout on the use of a grant entry and take necessary
>>>  *  action. (NB. This is not yet implemented!).
>>>  *
>>>  * Invalidating an unused GTF_accept_transfer entry:
>>> + * @code
>>>  *  1. flags = ent->flags.
>>>  *  2. Observe that !(flags & GTF_transfer_committed). [*]
>>>  *  3. Check result of SMP-safe CMPXCHG(&ent->flags, flags, 0).
>>
>> Since neither in the cover letter nor in the description here I could
>> spot a link to the resulting generated doc, I wonder what the
>> inconsistencies above are about: You add @code/@endcode (and no blank
>> lines) to parts of what's being described, and _instead_ a blank line
>> to another part. I think the goal should be that not only the
>> generated doc ends up looking "nice", but that the source code also
>> remains consistent. Otherwise, someone like me coming across this
>> later on might easily conclude that there was a @code/@endcode pair
>> missed.
> 
> Yes I’ll explain better in the commit message, that part and other parts are
> enclosed by @code/@endcode because they are formatted using spaces.
> If the block is not enclosed the spaces are missing in the html page resulting
> In a mess.
> If you can suggest an alias for the @code/@endcode command, I can create
> it so that the user looking the source code can understand better what's 
> going on.
> An example: @keepformat/@endkeepformat OR @keepindent/@endkeepindent

Oh, are you suggesting @code / @endcode isn't something doxygen mandates?
In this case either of your alternative suggestions would look better to
me. Which one depend on whether the goal to to merely keep indentation or
whether formatting should be kept altogether.

>>> @@ -97,18 +104,23 @@
>>>  *      transferred frame is written. It is safe for the guest to spin 
>>> waiting
>>>  *      for this to occur (detect by observing GTF_transfer_completed in
>>>  *      ent->flags).
>>> + * @endcode
>>>  *
>>>  * Invalidating a committed GTF_accept_transfer entry:
>>>  *  1. Wait for (ent->flags & GTF_transfer_completed).
>>>  *
>>
>> Why did this not also get enclosed by @code/@endcode?
> 
> In this case there are no spaces that contributes to the indentation.

But if, for consistency, @code / @endcode were added here, all would
still be well?

>>>  * Changing a GTF_permit_access from writable to read-only:
>>> + *
>>>  *  Use SMP-safe CMPXCHG to set GTF_readonly, while checking !GTF_writing.
>>>  *
>>>  * Changing a GTF_permit_access from read-only to writable:
>>> + *
>>>  *  Use SMP-safe bit-setting instruction.
>>
>> And these two?
> 
> These two lines makes the resulting html page looks better, the source code 
> however
> seems not too impacted by the change though.

I was rather asking about the absence of @code / @endcode here.

>>> + * @addtogroup grant_table Grant Tables
>>
>> And no blank (comment) line ahead of this?
> 
> Here there is no need for a blank line in the comment, but if in your opinion 
> the source
> code will look better I can add it

I think so, yes.

>>> @@ -120,24 +132,26 @@ typedef uint32_t grant_ref_t;
>>>  * [GST]: This field is written by the guest and read by Xen.
>>>  */
>>>
>>> -/*
>>> - * Version 1 of the grant table entry structure is maintained purely
>>> - * for backwards compatibility.  New guests should use version 2.
>>> - */
>>> #if __XEN_INTERFACE_VERSION__ < 0x0003020a
>>> #define grant_entry_v1 grant_entry
>>> #define grant_entry_v1_t grant_entry_t
>>> #endif
>>> +/**
>>> + * Version 1 of the grant table entry structure is maintained purely
>>> + * for backwards compatibility.  New guests should use version 2.
>>> + */
>>
>> In case I didn't say so already before - I think this would be a good
>> opportunity to drop the comment pointing at v2. With v2 optionally
>> unavailable altogether, this can't possibly be a generally valid
>> course of action.
> 
> Could you explain me better that? Do you want to get rid of this comment?

Especially the second sentence is misleading. If new code used v2, it
would not work on Xen with v2 support turned off.

> /**
> * Version 1 of the grant table entry structure is maintained purely
> * for backwards compatibility.  New guests should use version 2.
> */
> 
> In this case I don’t think this commit is the right place to do that, I can 
> just
> put it back where it was so that the documentation simply doesn’t show that.

Keeping misleading information out of the docs is imo rather desirable.
Of course we should, independently of that, also address the misleading
information in the source code. I can accept that doing the adjustment
right in this patch may not be ideal. I don't suppose I could talk you
into adding a prereq patch dropping at least that 2nd sentence?

Jan