Re: [Xen-devel] [PATCH v2 4/5] xen/gnttab: Refactor gnttab_clear_flag() to be gnttab_clear_flags()

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andrew,

On 7/8/19 9:21 PM, Andrew Cooper wrote:
> On 08/07/2019 20:27, Julien Grall wrote:
> > Hi,
> >
> > On 7/8/19 7:11 PM, Andrew Cooper wrote:
> > > On 07/07/2019 19:42, Julien Grall wrote:
> > > > Hi Andrew,
> > > >
> > > > On 7/4/19 8:14 PM, Andrew Cooper wrote:
> > > > > To allow for further improvements, it is useful to be able to clear
> > > > > more than
> > > > > a single flag at once.  Rework gnttab_clear_flag() into
> > > > > gnttab_clear_flags()
> > > > > which takes a bitmask rather than a bit number.
> > > > >
> > > > > No practical change yet.
> > > > >
> > > > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > > > ---
> > > > > CC: Jan Beulich <JBeulich@xxxxxxxx>
> > > > > CC: Wei Liu <wl@xxxxxxx>
> > > > > CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > > > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > > CC: Julien Grall <julien.grall@xxxxxxx>
> > > > > CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > > > >
> > > > > v2:
> > > > >     * Use unsigned int for the mask parameter
> > > > I don't seem to find the request on the ML. Technically the mask can
> > > > only be 16-bit. May I ask the reason of this change?
> > > It is on the mailing list, but an orphaned email due to Jan's email
> > > changes.
> > Is it the same problem as I have seen the past 6 months between
> > Juergen and Jan's e-mail?
> I think its different, but I'm losing track tbh.
It would be nice to resolve it... It is a pain to try to match them with 
the correct thread.
> > > https://lore.kernel.org/xen-devel/1561109798-8744-5-git-send-email-andrew.cooper3@xxxxxxxxxx/T/#t
> > To be honest, I think it is wrong to try to micro-optimize a common
> > prototype for the benefit of one architecture and one compiler version
> > (or even none per the e-mail).
> The prototype wasn't common.  Observe that before this patch, ARM used
> unsigned long while x86 used uint16_t.  It should become common, however.
I am not sure to follow this. AFAICT, we use uint16_t properly on Arm.

> In practice, we're talking about bits 3 and 4, and this isn't liable to
> change in a hurry.
>
> > One could also argue that this may be not beneficial for the non-x86
> > architecture depending on how the compiler decide to do the cast from
> > 32-bit to 16-bit...
> All architecture necessarily suffer the downcast somewhere, even x86.
> ARM's is in the prototype for guest_clear_mask16(), but in terms of the
> common logic for calculating conditionally which bits to clear, keeping
> everything as unsigned int for as long as possible offers the most
> flexibility to the compiler, as it can see all the constants involved.
This is the argument I was looking for :). Thank you for writing it!

Cheers,

--
Julien Grall

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