From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:44463) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hIgEF-0002SS-BU for qemu-devel@nongnu.org; Mon, 22 Apr 2019 17:11:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hIg6f-0008LI-VG for qemu-devel@nongnu.org; Mon, 22 Apr 2019 17:04:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48632) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1hIg6f-0008Kg-Mx for qemu-devel@nongnu.org; Mon, 22 Apr 2019 17:04:05 -0400 From: Jeff Moyer References: <20190410040826.24371-1-pagupta@redhat.com> <20190410040826.24371-2-pagupta@redhat.com> <20190412083230.GA29850@quack2.suse.cz> <20190418161833.GA22970@infradead.org> Date: Mon, 22 Apr 2019 17:03:50 -0400 In-Reply-To: (Dan Williams's message of "Mon, 22 Apr 2019 12:44:21 -0700") Message-ID: MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 1/6] libnvdimm: nd_region flush callback support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dan Williams Cc: Christoph Hellwig , Jan Kara , Pankaj Gupta , linux-nvdimm , Linux Kernel Mailing List , virtualization@lists.linux-foundation.org, KVM list , linux-fsdevel , Linux ACPI , Qemu Developers , linux-ext4 , linux-xfs , Ross Zwisler , Vishal L Verma , Dave Jiang , "Michael S. Tsirkin" , Jason Wang , Matthew Wilcox , "Rafael J. Wysocki" , Len Brown , Theodore Ts'o , Andreas Dilger , "Darrick J. Wong" , lcapitulino@redhat.com, Kevin Wolf , Igor Mammedov , Nitesh Narayan Lal , Rik van Riel , Stefan Hajnoczi , Andrea Arcangeli , David Hildenbrand , david , cohuck@redhat.com, Xiao Guangrong , Paolo Bonzini , kilobyte@angband.pl, yuval shaia Dan Williams writes: > On Mon, Apr 22, 2019 at 8:59 AM Jeff Moyer wrote: >> >> Dan Williams writes: >> >> > On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig wrote: >> >> >> >> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote: >> >> > > > I'd either add a comment about avoiding retpoline overhead here or just >> >> > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't >> >> > > > get confused by the code. >> >> > > >> >> > > Isn't this premature optimization? I really don't like adding things >> >> > > like this without some numbers to show it's worth it. >> >> > >> >> > I don't think it's premature given this optimization technique is >> >> > already being deployed elsewhere, see: >> >> > >> >> > https://lwn.net/Articles/774347/ >> >> >> >> For one this one was backed by numbers, and second after feedback >> >> from Linux we switched to the NULL pointer check instead. >> > >> > Ok I should have noticed the switch to NULL pointer check. However, >> > the question still stands do we want everyone to run numbers to >> > justify this optimization, or make it a new common kernel coding >> > practice to do: >> > >> > if (!object->op) >> > generic_op(object); >> > else >> > object->op(object); >> > >> > ...in hot paths? >> >> I don't think nvdimm_flush is a hot path. Numbers of some >> representative workload would prove one of us right. > > I'd rather say that the if "if (!op) do_generic()" pattern is more > readable in the general case, saves grepping for who set the op in the > common case. The fact that it has the potential to be faster is gravy > at that point. If the primary motivation is performance, then I'd expect performance numbers to back it up. If that isn't the primary motivation, then choose whichever way you feel is appropriate. Cheers, Jeff 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 X-Spam-Level: X-Spam-Status: No, score=-0.9 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A2620C10F11 for ; Mon, 22 Apr 2019 21:16:56 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6B5A320896 for ; Mon, 22 Apr 2019 21:16:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6B5A320896 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([127.0.0.1]:44361 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hIgJ5-0005zC-Ls for qemu-devel@archiver.kernel.org; Mon, 22 Apr 2019 17:16:55 -0400 Received: from eggs.gnu.org ([209.51.188.92]:44463) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hIgEF-0002SS-BU for qemu-devel@nongnu.org; Mon, 22 Apr 2019 17:11:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hIg6f-0008LI-VG for qemu-devel@nongnu.org; Mon, 22 Apr 2019 17:04:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48632) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1hIg6f-0008Kg-Mx for qemu-devel@nongnu.org; Mon, 22 Apr 2019 17:04:05 -0400 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4E1C5882EA; Mon, 22 Apr 2019 21:04:03 +0000 (UTC) Received: from segfault.boston.devel.redhat.com (segfault.boston.devel.redhat.com [10.19.60.26]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1D7F260141; Mon, 22 Apr 2019 21:03:51 +0000 (UTC) From: Jeff Moyer To: Dan Williams References: <20190410040826.24371-1-pagupta@redhat.com> <20190410040826.24371-2-pagupta@redhat.com> <20190412083230.GA29850@quack2.suse.cz> <20190418161833.GA22970@infradead.org> X-PGP-KeyID: 1F78E1B4 X-PGP-CertKey: F6FE 280D 8293 F72C 65FD 5A58 1FF8 A7CA 1F78 E1B4 Date: Mon, 22 Apr 2019 17:03:50 -0400 In-Reply-To: (Dan Williams's message of "Mon, 22 Apr 2019 12:44:21 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 22 Apr 2019 21:04:04 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: Re: [Qemu-devel] [PATCH v5 1/6] libnvdimm: nd_region flush callback support X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Pankaj Gupta , cohuck@redhat.com, Jan Kara , KVM list , "Michael S. Tsirkin" , Jason Wang , david , Qemu Developers , virtualization@lists.linux-foundation.org, Andreas Dilger , Ross Zwisler , Andrea Arcangeli , Dave Jiang , linux-nvdimm , Vishal L Verma , David Hildenbrand , Matthew Wilcox , Christoph Hellwig , Linux ACPI , linux-ext4 , Len Brown , kilobyte@angband.pl, Rik van Riel , yuval shaia , Stefan Hajnoczi , Paolo Bonzini , lcapitulino@redhat.com, Kevin Wolf , Nitesh Narayan Lal , Theodore Ts'o , Xiao Guangrong , "Darrick J. Wong" , "Rafael J. Wysocki" , Linux Kernel Mailing List , linux-xfs , linux-fsdevel , Igor Mammedov Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Message-ID: <20190422210350.aB2wEdlmP7T9UWX1LKTqjljWEWyTaT3V1mLFX6fuBBw@z> Dan Williams writes: > On Mon, Apr 22, 2019 at 8:59 AM Jeff Moyer wrote: >> >> Dan Williams writes: >> >> > On Thu, Apr 18, 2019 at 9:18 AM Christoph Hellwig wrote: >> >> >> >> On Thu, Apr 18, 2019 at 09:05:05AM -0700, Dan Williams wrote: >> >> > > > I'd either add a comment about avoiding retpoline overhead here or just >> >> > > > make ->flush == NULL mean generic_nvdimm_flush(). Just so that people don't >> >> > > > get confused by the code. >> >> > > >> >> > > Isn't this premature optimization? I really don't like adding things >> >> > > like this without some numbers to show it's worth it. >> >> > >> >> > I don't think it's premature given this optimization technique is >> >> > already being deployed elsewhere, see: >> >> > >> >> > https://lwn.net/Articles/774347/ >> >> >> >> For one this one was backed by numbers, and second after feedback >> >> from Linux we switched to the NULL pointer check instead. >> > >> > Ok I should have noticed the switch to NULL pointer check. However, >> > the question still stands do we want everyone to run numbers to >> > justify this optimization, or make it a new common kernel coding >> > practice to do: >> > >> > if (!object->op) >> > generic_op(object); >> > else >> > object->op(object); >> > >> > ...in hot paths? >> >> I don't think nvdimm_flush is a hot path. Numbers of some >> representative workload would prove one of us right. > > I'd rather say that the if "if (!op) do_generic()" pattern is more > readable in the general case, saves grepping for who set the op in the > common case. The fact that it has the potential to be faster is gravy > at that point. If the primary motivation is performance, then I'd expect performance numbers to back it up. If that isn't the primary motivation, then choose whichever way you feel is appropriate. Cheers, Jeff