From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752298AbdFTUay (ORCPT ); Tue, 20 Jun 2017 16:30:54 -0400 Received: from mail-yb0-f180.google.com ([209.85.213.180]:34483 "EHLO mail-yb0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751029AbdFTUaw (ORCPT ); Tue, 20 Jun 2017 16:30:52 -0400 Date: Tue, 20 Jun 2017 16:30:49 -0400 From: Tejun Heo To: Nikolay Borisov Cc: jbacik@fb.com, linux-kernel@vger.kernel.org, mgorman@techsingularity.net Subject: Re: [PATCH v2 2/2] writeback: Rework wb_[dec|inc]_stat family of functions Message-ID: <20170620203049.GH21326@htj.duckdns.org> References: <20170620173346.GB21326@htj.duckdns.org> <1497981720-7036-1-git-send-email-nborisov@suse.com> <20170620193715.GF21326@htj.duckdns.org> <274063e4-57d0-5a87-1f43-28f5232af52b@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <274063e4-57d0-5a87-1f43-28f5232af52b@suse.com> User-Agent: Mutt/1.8.2 (2017-04-18) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, On Tue, Jun 20, 2017 at 11:28:30PM +0300, Nikolay Borisov wrote: > > Heh, looks like I was confused. __percpu_counter_add() is not > > irq-safe. It disables preemption and uses __this_cpu_read(), so > > there's no protection against irq. If writeback statistics want > > irq-safe operations and it does, it would need these separate > > operations. Am I missing something? > > So looking at the history of the commit initially there was > preempt_disable + this_cpu_ptr which was later changed in: > > 819a72af8d66 ("percpucounter: Optimize __percpu_counter_add a bit > through the use of this_cpu() options.") > > I believe that having __this_cpu_read ensures that we get an atomic > snapshot of the variable but when we are doing the actual write e.g. the > else {} branch we actually use this_cpu_add which ought to be preempt + > irq safe, meaning we won't get torn write. In essence we have atomic > reads by merit of __this_cpu_read + atomic writes by merit of using > raw_spin_lock_irqsave in the if() branch and this_cpu_add in the else {} > branch. Ah, you're right. The initial read is speculative. The slow path is protected with irq spinlock. The fast path is this_cpu_add() which is irq-safe. We really need to document these functions. Can I bother you with adding documentation to them while you're at it? Thanks. -- tejun