[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 8/8] x86/PV: use get_unsafe() instead of copy_from_unsafe()

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 23 Feb 2021 16:37:15 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=dk0QKkx8KOD+aEZhcQ5FynwWqr2Jayi3cggBH2hmmCM=; b=V6inC/Et0GBzG1ABQ1OEV2dRLX2Sc6wSp7dOoj7sNypDSREFOVhRuEZHK7fRhzJCp326Guwr79XKuPLHUJxJHV5+QMdByzOsFevDGnQaV5/U4Q2+8wOXlyZWw50RqPg3kSKvEOmwhBWEEppEcaKzMVEdQrXBPaXZqZr+yc2tVhnx8hvC144IdX0wvGS83wm+mDS3p+KwRgE8tXyajZSfI/74gsKpCUzOZoXmPM7rxQZgCMdyHMGfueEW4rax35sTgqLD3TcRe9Snd+cd8/yByWsTtrygqoAsYArqQRHAa3cKJk0oSB/c2oBl3xXgBYghlY6TbJi4j3tyVKl78mlFOw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=eqR3G8/dECBEEf55WQyQa0klKxFqoXJEl9YultfD8ub7hCyo2N4jMu6yFEaWMLedxE23cYv89pB4Lc0Gy2cZpEgHf2aDacLU3GTW07zg6FmHbr17B7JevQUJS8ycUFvYstRFCqb4XCIbaNaBMyTEKgWhOXHGgj+GZWNJfRbOgu02gqmZzzXxbPmjYYpZU3DR8s30zoQP2pQjPm3+pwLq821KnjNxEo7YCwkTwkD0j2f4K7z9fb2IGp04iLtn9Jtnu3M0RkEVX9ogl6V4W6jVshgA8NJ0F8YKzsDtI46NaQjAZS/6Htq8AUJxDc0toAJwfXQOMvktjAl+LRg6bJvJqw==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Andrew Cooper" <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>
  • Delivery-date: Tue, 23 Feb 2021 15:37:38 +0000
  • Ironport-sdr: nmcTU305/hoj0tgouqRSvlVG/cJF2Kk1n4OVKwoiyp8jbcI28qefgm57HqqhkWL/AhRwYPg2zj H14Mk3kl7+Ju7FGF6ECNUyQbznuzeGhs/7EPaj8GCvVljCKbnImxZe6S+2u+NKsj5/ke3jZ/La DD80EiaKPka4VKAwtTMzWgEgZt/ewmjeIVbskfGowh2u6ooJZiUtgXDmBlq+BsS2TIdwkPoF80 L1nOZqMiVvGWdWdgdnSLgGIgk47INbUBZfkjABOChneQhOHhyXe6xQ7uExGYjad/OEFe437V+f HoQ=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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 <jbeulich@xxxxxxxx>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > 
> > 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.

Thanks, Roger.



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.