Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756216AbcJPWtF (ORCPT ); Sun, 16 Oct 2016 18:49:05 -0400 Received: from mail-lf0-f50.google.com ([209.85.215.50]:33097 "EHLO mail-lf0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752492AbcJPWs6 (ORCPT ); Sun, 16 Oct 2016 18:48:58 -0400 MIME-Version: 1.0 In-Reply-To: <1476655575-6588-1-git-send-email-joelaf@google.com> References: <20161016061057.GA26990@infradead.org> <1476655575-6588-1-git-send-email-joelaf@google.com> From: Joel Fernandes Date: Sun, 16 Oct 2016 15:48:42 -0700 Message-ID: Subject: Re: [PATCH v3] mm: vmalloc: Replace purge_lock spinlock with atomic refcount To: Christoph Hellwig Cc: LKML , "open list:MEMORY MANAGEMENT" , linux-rt-users@vger.kernel.org, Joel Fernandes , Chris Wilson , Jisheng Zhang , John Dias , Andrew Morton Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2748 Lines: 61 Hi Christoph, On Sun, Oct 16, 2016 at 3:06 PM, Joel Fernandes wrote: > On Sat, Oct 15, 2016 at 11:10 PM, Christoph Hellwig wrote: >> On Sat, Oct 15, 2016 at 03:59:34PM -0700, Joel Fernandes wrote: >>> Also, could you share your concerns about use of atomic_t in my patch? >>> I believe that since this is not a contented variable, the question of >>> lock fairness is not a concern. It is also not a lock really the way >>> I'm using it, it just keeps track of how many purges are in progress.. >> >> atomic_t doesn't have any acquire/release semantics, and will require >> off memory barrier dances to actually get the behavior you intended. >> And from looking at the code I can't really see why we even would >> want synchronization behavior - for the sort of problems where we >> don't want multiple threads to run the same code at the same time >> for effiency but not correctness reasons it's usually better to have >> batch thresholds and/or splicing into local data structures before >> operations. Both are techniques used in this code, and I'd rather >> rely on them and if required improve on them then using very odd >> hoc synchronization methods. > > Thanks for the explanation. If you know of a better way to handle the sync=1 > case, let me know. In defense of atomics, even vmap_lazy_nr in the same code is > atomic_t :) I am also not using it as a lock really, but just to count how many > times something is in progress that's all - I added some more comments to my > last patch to make this clearer in the code and now I'm also handling the case > for sync=1. Also, one more thing about the barrier dances you mentioned, this will also be done by the spinlock which was there before my patch. So in favor of my patch, it doesn't make things any worse than they were and actually fixes the reported issue while preserving the original code behavior. So I think it is a good thing to fix the issue considering so many people are reporting it and any clean ups of the vmalloc code itself can follow. If you want I can looking into replacing the atomic_cmpxchg with an atomic_inc and not do anything different for sync vs !sync except for spinning when purges are pending. Would that make you feel a bit better? So instead of: if (!sync && !force_flush) { /* * Incase a purge is already in progress, just return. */ if (atomic_cmpxchg(&purging, 0, 1)) return; } else atomic_inc(&purging); , Just do a: atomic_inc(&purging); This should be Ok to do since in the !sync case, we'll just return anyway if another purge was in progress. Thanks, Joel