From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 88108C433F5 for ; Wed, 12 Jan 2022 11:35:12 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D4DE26B0146; Wed, 12 Jan 2022 06:35:11 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id CFCF36B0147; Wed, 12 Jan 2022 06:35:11 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BC4296B0148; Wed, 12 Jan 2022 06:35:11 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0048.hostedemail.com [216.40.44.48]) by kanga.kvack.org (Postfix) with ESMTP id AA9336B0146 for ; Wed, 12 Jan 2022 06:35:11 -0500 (EST) Received: from smtpin26.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 6A25B8FD05 for ; Wed, 12 Jan 2022 11:35:11 +0000 (UTC) X-FDA: 79021428822.26.B21FD80 Received: from mail-vk1-f181.google.com (mail-vk1-f181.google.com [209.85.221.181]) by imf29.hostedemail.com (Postfix) with ESMTP id 8113D120019 for ; Wed, 12 Jan 2022 11:35:09 +0000 (UTC) Received: by mail-vk1-f181.google.com with SMTP id bj47so1409022vkb.13 for ; Wed, 12 Jan 2022 03:35:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9fTJTS7bqQi/yIpHQJpi+4PrBqmFnBRpYezv+IVLW9I=; b=PJMZnFO3s5Vo9SbANIVxOvoQeetaMi9gbFjbdqqF4JSRyWYh+1M91PRfMqEmTQ78e5 OnKctDFWn1mpr89a3TweLsKvM3kV9qxcepvL3I4uqq6a/OBFbH2giL/R1IKgJr+g2kto 4SH+6WRJWSvwhJHBKPSDg0GM+kPh7JIixDJSJZ0K11PSe42JaW31WoEZHELzoqt3y7K0 yi7geIUn7EcZKWJLr1IWrhGp1aPEne8wzRAo6PNpR6KmGwzepSPADJxidiHep8Td/T5O cyxYmUVJcja6dsjLnGSszJGaVbeGdhSB1pTzXp89DpMYx9qfwOlnCh9RWrZs+hA7ddQf m4iQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9fTJTS7bqQi/yIpHQJpi+4PrBqmFnBRpYezv+IVLW9I=; b=VXBGfHvo3kZvwu+powJT5/P4LlekggdkvAuiI2pbsWLIgDwVEJivYXx2HqJ9Tpoy6I KPwwS3c5Tb8d4vMgC2wp5T/KYt7ItWnJLWzVI0bS0imSSSiXEPQ9neEhNu9Ac1Pd7WNd IQc0nYRLEWKRm87qxBm1yk6kmgr4udLACEUm/r5UuLz0drod6efEQhn29QO2cQgZ1uZl JenOSXWb6OvlWbTINo2cMXIueT6GVKCt2UL2CCvpPzE5XfJHbkLyCy64AXGv1UcV8Co/ VQ8CFGBCdeCrMN0Ykhv5s5cTKUirF+/WbtH7jSwJQYOSO30b6jVWUlgwn/pZ7LY/eIyt Cc1g== X-Gm-Message-State: AOAM532AblELyP3raq0/WOJ66k56kGyIbMk69wLlmYvnAxXdwrye7Xtr 3ZALQU6D38dRvRAsQ92IzWxah8WP+RdunOjAge4= X-Google-Smtp-Source: ABdhPJz/bwCv+8MKvrlj91tEjjDwA9AJb+uzqY+ZGTKpvabwgJ2a1V7vqHWmYdL/yvJe0UT5JAPtwNim/8kwef6uymU= X-Received: by 2002:a05:6122:130e:: with SMTP id e14mr4494396vkp.41.1641987309469; Wed, 12 Jan 2022 03:35:09 -0800 (PST) MIME-Version: 1.0 References: <1641488717-13865-1-git-send-email-quic_charante@quicinc.com> <2c66ba2e-1c65-3bdd-b91e-eb8391ec6dbf@quicinc.com> <61212ffd-3505-04d7-5403-932a10d676e7@quicinc.com> In-Reply-To: <61212ffd-3505-04d7-5403-932a10d676e7@quicinc.com> From: Mark Hemment Date: Wed, 12 Jan 2022 11:34:58 +0000 Message-ID: Subject: Re: [PATCH v3 RESEND] mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED for shmem To: Charan Teja Kalla Cc: Hugh Dickins , Andrew Morton , "Matthew Wilcox (Oracle)" , vbabka@suse.cz, rientjes@google.com, mhocko@suse.com, Suren Baghdasaryan , Shakeel Butt , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Charan Teja Reddy Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 8113D120019 X-Stat-Signature: h4g6pnjs6t1hrmnkgkjpuguzratptfsh Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=googlemail.com header.s=20210112 header.b=PJMZnFO3; dmarc=pass (policy=quarantine) header.from=googlemail.com; spf=pass (imf29.hostedemail.com: domain of markhemm@googlemail.com designates 209.85.221.181 as permitted sender) smtp.mailfrom=markhemm@googlemail.com X-Rspamd-Server: rspam08 X-HE-Tag: 1641987309-78364 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000001, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, 12 Jan 2022 at 08:22, Charan Teja Kalla wrote: > > Hello Mark, > > On 1/10/2022 3:51 PM, Charan Teja Kalla wrote: > >>> +static int shmem_fadvise_willneed(struct address_space *mapping, > >>> + pgoff_t start, pgoff_t long end) > >>> +{ > >>> + XA_STATE(xas, &mapping->i_pages, start); > >>> + struct page *page; > >>> + > >>> + rcu_read_lock(); > >>> + xas_for_each(&xas, page, end) { > >>> + if (!xa_is_value(page)) > >>> + continue; > >>> + xas_pause(&xas); > >>> + rcu_read_unlock(); > >>> + > >>> + page = shmem_read_mapping_page(mapping, xas.xa_index); > >>> + if (!IS_ERR(page)) > >>> + put_page(page); > >>> + > >>> + rcu_read_lock(); > >>> + if (need_resched()) { > >>> + xas_pause(&xas); > >>> + cond_resched_rcu(); > >>> + } > >>> + } > >>> + rcu_read_unlock(); > >>> + > >>> + return 0; > >> I have a doubt on referencing xa_index after calling xas_pause(). > >> xas_pause() walks xa_index forward, so will not be the value expected > >> for the current page. > > Agree here. I should have the better test case to verify my changes. > > > >> Also, not necessary to re-call xas_pause() before cond_resched (it is > >> a no-op). > > In the event when CONFIG_DEBUG_ATOMIC_SLEEP is enabled users may still > > need to call the xas_pause(), as we are dropping the rcu lock. NO? > > > > static inline void cond_resched_rcu(void) > > { > > #if defined(CONFIG_DEBUG_ATOMIC_SLEEP) || !defined(CONFIG_PREEMPT_RCU) > > rcu_read_unlock(); > > cond_resched(); > > rcu_read_lock(); > > #endif > > } > > > >> Would be better to check need_resched() before > >> rcu_read_lock(). > > Okay, I can directly use cond_resched() if used before rcu_read_lock(). > > > >> As this loop may call xas_pause() for most iterations, should consider > >> using xa_for_each() instead (I *think* - still getting up to speed > >> with XArray). > > Even the xarray documentation says that: If most entries found during a > > walk require you to call xas_pause(), the xa_for_each() iterator may be > > more appropriate. > > > > Since every value entry found in the xarray requires me to do the > > xas_pause(), I do agree that xa_for_each() is the appropriate call here. > > Will switch to this in the next spin. Waiting for further review > > comments on this patch. > > I also found the below documentation: > xa_for_each() will spin if it hits a retry entry; if you intend to see > retry entries, you should use the xas_for_each() iterator instead. > > Since retry entries are expected, I should be using the xas_for_each() > with the corrections you had pointed out. Isn't it? > Ah, you've hit a barrier on my Xarray knowledge. The current shmem code simply does a 'continue' on xas_retry(). Is this different from Xarray looping internally for xas_retry()? I assume not, but cannot give an definite answer (sorry). Cheers, Mark