From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f202.google.com (mail-pl1-f202.google.com [209.85.214.202]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 77ACD136E3F for ; Fri, 30 Aug 2024 03:48:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.202 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724989683; cv=none; b=VRxXRDES+B6TZorSnbr3JWQgyqoxI0M7SBS9/KP6GB8pp+I7jfS6j+nNxSDAoRy4q3eFXswkT8BREL2NvEfcx7tM7XhXh5P0BRLKA/Z2fHTotV8xuA0NhhmgyN5By27ig+vNPla/8R/hl1IA6ro91/GO9cr7Hu9RXsEMzRZYKaQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724989683; c=relaxed/simple; bh=8X4cJIJqX+703YojJTQK5ekGonK/y/arTKL/aqKkyEI=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=lzvuCYoyxicNrKJNKIiHOitBuR/e+mVyXGXvuVM1GTjyJNVCpzVYVLlq279eu4wdjgE2j1TKKlSdzfTSZzSaiyfj5Xc8J/EOxAhPVMt1ZM7QzrxHSG+OEBghvd2zTplrDqPG8vDs5LNW1ctxNtsgakMBaRiajMx5wxv83gtVj8U= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--seanjc.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=GrTw6i/H; arc=none smtp.client-ip=209.85.214.202 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--seanjc.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="GrTw6i/H" Received: by mail-pl1-f202.google.com with SMTP id d9443c01a7336-201f464e3e8so14664065ad.3 for ; Thu, 29 Aug 2024 20:48:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1724989681; x=1725594481; darn=lists.linux.dev; h=content-transfer-encoding:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:from:to:cc:subject:date:message-id :reply-to; bh=7oDkTMtkbW8nQHJkC6FNRU/4bZFr8mldBmCcZx8NdnI=; b=GrTw6i/H3tZdlJVGCbAhKQllnCT+KPY1FIJv1gyb3vmoM/KJMR0y6OgkXQ8UFsK6Xr 14Ab9s8jeAuAT87hXIEdWg7C3F9y7o768CvpaEg+lDyOpequxTR1gYVZ9EZpkGFIBBGr dInz1uzqt8RDQ1D5eccPBzm6sQWW6M/NCM0V/EA8XRFin6Y+q/fIsghCHz6BKmfTdRiH lvKlgGWyA3aLF1B/yxZuPlbSyyNAbh8lORzWpO2PmHr1XRhYfOxZuxU+P60h/J7mPFTT 1ZKEXwSMcsE1rE7HUyQxy0nHx3HZhSrjznwiWzCmcKS3TwBDkjRRndFmSFK85ZdT7sIz x41Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724989681; x=1725594481; h=content-transfer-encoding:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=7oDkTMtkbW8nQHJkC6FNRU/4bZFr8mldBmCcZx8NdnI=; b=W6WqJsx9GgOfGBB3r7wfzp7S/lIs/F8WKiM/7fdEvXNAg8l7EWhCiPOSUOmYX1t7OK ToB/bTyFh3hz8hgAzP5k5reAnMhxoKdvwgxF2WVnp3yAedXdZgnUICoRG2BrYSErS+IG TXRy2rnPZ00H8morLKSlxng/OkI9QCbE+2vKTs13gThpVdXqeh4I8WURDP/buUY8IaII Ayn0hwZVYituAfbQP9fHOkBpY+FOzk/hl9byu6ESQYjaxNA7M4J7jL70e/XBTFdKSTWt UmeLHz5CcEFIAfSV+z6z8k9PP2VUTL2rnQW23YU5iLJlmc+QK85ii/IPpBQFOyW6Iqux zY2w== X-Forwarded-Encrypted: i=1; AJvYcCUC87GgwiqcXQJlyUlCmkNmQV4aWfUousnYD0YEe2myZjw59jHO4Tk+LbbnX3NDVOfc0R+c824=@lists.linux.dev X-Gm-Message-State: AOJu0Ywds3QqOFD+MD3glp5NnaIcaMXGHzeXbN/zgn21OxcmkPgV1aDh i/r/Bp/nGajkdQiR1mmcFTubXO3t0HMRRHSrwm/vd+sH1fnVhzk36v6AvgZdWt8EEUnco5VpxD0 2Ig== X-Google-Smtp-Source: AGHT+IFuJaHoFSCV5+8BWRf+q8zKeUDxkgIb0cxDbWq2MCPl2PJvwzOB/7AGr0dtEc/kwU7osl3dzfbQCdc= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a17:902:f54c:b0:1ff:4618:36dd with SMTP id d9443c01a7336-20527612dc9mr665745ad.1.1724989680602; Thu, 29 Aug 2024 20:48:00 -0700 (PDT) Date: Thu, 29 Aug 2024 20:47:59 -0700 In-Reply-To: Precedence: bulk X-Mailing-List: kvmarm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20240724011037.3671523-1-jthoughton@google.com> <20240724011037.3671523-3-jthoughton@google.com> Message-ID: Subject: Re: [PATCH v6 02/11] KVM: x86: Relax locking for kvm_test_age_gfn and kvm_age_gfn From: Sean Christopherson To: James Houghton Cc: Andrew Morton , Paolo Bonzini , Ankit Agrawal , Axel Rasmussen , Catalin Marinas , David Matlack , David Rientjes , James Morse , Jason Gunthorpe , Jonathan Corbet , Marc Zyngier , Oliver Upton , Raghavendra Rao Ananta , Ryan Roberts , Shaoqin Huang , Suzuki K Poulose , Wei Xu , Will Deacon , Yu Zhao , Zenghui Yu , kvmarm@lists.linux.dev, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Thu, Aug 29, 2024, James Houghton wrote: > On Fri, Aug 16, 2024 at 6:05=E2=80=AFPM Sean Christopherson wrote: > > > +static __always_inline bool kvm_tdp_mmu_handle_gfn_lockless( > > > + struct kvm *kvm, > > > + struct kvm_gfn_range *range, > > > + tdp_handler_t handler) > > > > Please burn all the Google3 from your brain, and code ;-) >=20 > I indented this way to avoid going past the 80 character limit. I've > adjusted it to be more like the other functions in this file. >=20 > Perhaps I should put `static __always_inline bool` on its own line? Noooo. Do not wrap before the function name. Linus has a nice explanation/= rant on this[1]. In this case, I'm pretty sure you can avoid the helper and simply handle al= l aging paths in a single API, e.g. similar to what I proposed for the shadow MMU[2= ]. [1] https://lore.kernel.org/all/CAHk-=3DwjoLAYG446ZNHfg=3DGhjSY6nFmuB_wA8fY= d5iLBNXjo9Bw@mail.gmail.com [2] https://lore.kernel.org/all/20240809194335.1726916-16-seanjc@google.com > > > /* > > > * Mark the SPTEs range of GFNs [start, end) unaccessed and return n= on-zero > > > * if any of the GFNs in the range have been accessed. > > > @@ -1237,28 +1272,30 @@ static bool age_gfn_range(struct kvm *kvm, st= ruct tdp_iter *iter, > > > { > > > u64 new_spte; > > > > > > +retry: > > > /* If we have a non-accessed entry we don't need to change the = pte. */ > > > if (!is_accessed_spte(iter->old_spte)) > > > return false; > > > > > > if (spte_ad_enabled(iter->old_spte)) { > > > - iter->old_spte =3D tdp_mmu_clear_spte_bits(iter->sptep, > > > - iter->old_spte= , > > > - shadow_accesse= d_mask, > > > - iter->level); > > > + iter->old_spte =3D tdp_mmu_clear_spte_bits_atomic(iter-= >sptep, > > > + shadow_accessed_mask); > > > new_spte =3D iter->old_spte & ~shadow_accessed_mask; > > > } else { > > > - /* > > > - * Capture the dirty status of the page, so that it doe= sn't get > > > - * lost when the SPTE is marked for access tracking. > > > - */ > > > + new_spte =3D mark_spte_for_access_track(iter->old_spte)= ; > > > + if (__tdp_mmu_set_spte_atomic(iter, new_spte)) { > > > + /* > > > + * The cmpxchg failed. If the spte is still a > > > + * last-level spte, we can safely retry. > > > + */ > > > + if (is_shadow_present_pte(iter->old_spte) && > > > + is_last_spte(iter->old_spte, iter->level)) > > > + goto retry; > > > > Do we have a feel for how often conflicts actually happen? I.e. is it = worth > > retrying and having to worry about infinite loops, however improbable t= hey may > > be? >=20 > I'm not sure how common this is. I think it's probably better not to > retry actually. If the cmpxchg fails, this spte is probably young > anyway, so I can just `return true` instead of potentially retrying. > This is all best-effort anyway. +1