On Tue, Feb 23, 2021 at 05:13:21PM +0100, Jan Beulich wrote:
> On 23.02.2021 16:37, Roger Pau Monné wrote:
> > On Tue, Feb 23, 2021 at 04:25:00PM +0100, Jan Beulich wrote:
> >> On 23.02.2021 12:59, Roger Pau Monné wrote:
> >>> On Wed, Feb 17, 2021 at 09:23:33AM +0100, Jan Beulich wrote:
> >>>> The former expands to a single (memory accessing) insn, which the latter
> >>>> does not guarantee. Yet we'd prefer to read consistent PTEs rather than
> >>>> risking a split read racing with an update done elsewhere.
> >>>>
> >>>> Signed-off-by: Jan Beulich <jbeul...@suse.com>
> >>>
> >>> Reviewed-by: Roger Pau Monné <roger....@citrix.com>
> >>>
> >>> Albeit I wonder why the __builtin_constant_p check done in
> >>> copy_from_unsafe is not enough to take the get_unsafe_size branch in
> >>> there. Doesn't sizeof(l{1,2}_pgentry_t) qualify as a built time
> >>> constant?
> >>>
> >>> Or the fact that n it's a parameter to an inline function hides this,
> >>> in which case the __builtin_constant_p is pointless?
> >>
> >> Without (enough) optimization, __builtin_constant_p() may indeed
> >> yield false in such cases. But that wasn't actually what I had
> >> in mind when making this change (and the original similar on in
> >> shadow code). Instead, at the time I made the shadow side change,
> >> I had removed this optimization from the new function flavors.
> >> With that removal, things are supposed to still be correct - it's
> >> an optimization only, after all. Meanwhile the optimizations are
> >> back, so there's no immediate problem as long as the optimizer
> >> doesn't decide to out-of-line the function invocations (we
> >> shouldn't forget that even always_inline is not a guarantee for
> >> inlining to actually occur).
> >
> > I'm fine with you switching those use cases to get_unsafe, but I think
> > the commit message should be slightly adjusted to notice that
> > copy_from_unsafe will likely do the right thing, but that it's simply
> > clearer to call get_unsafe directly, also in case copy_from_unsafe
> > gets changed in the future to drop the get_unsafe paths.
>
> How about this then?
>
> "The former expands to a single (memory accessing) insn, which the latter
> does not guarantee (the __builtin_constant_p() based switch() statement
> there is just an optimization). Yet we'd prefer to read consistent PTEs
> rather than risking a split read racing with an update done elsewhere."
LGTM, thanks.
Roger.
