From owner-svn-src-all@freebsd.org Tue Feb 7 15:31:02 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B4115CD55AB for ; Tue, 7 Feb 2017 15:31:02 +0000 (UTC) (envelope-from steven.hartland@multiplay.co.uk) Received: from mail-wr0-x232.google.com (mail-wr0-x232.google.com [IPv6:2a00:1450:400c:c0c::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4BF251C94 for ; Tue, 7 Feb 2017 15:31:02 +0000 (UTC) (envelope-from steven.hartland@multiplay.co.uk) Received: by mail-wr0-x232.google.com with SMTP id i10so41014211wrb.0 for ; Tue, 07 Feb 2017 07:31:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=multiplay-co-uk.20150623.gappssmtp.com; s=20150623; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to; bh=14icftTA0jvXa4Tee8+dRHyqAezOrcis2S0LKdxY/6o=; b=XoU42eWrLPr62gU3IAalyBRfkqlnEUhs/3A4jO0H3LB+xfbcpaUMh/y3tIOcscSpf3 wlcUvJi0mFbxhy9bmO+8IfnasoDMXAPN2oXsTzB6dbtJSuXV+5dvzT7LUxzZQw/iuP1b iw2Ep8hQj7w8RFoUrEYPIfAGxsa2w7yVJkf2E8yb/mEAVwQk1/H3iTlsiY0bCplafg8t ckIHal6MC2nuthdrwtqfvVPZA/I77J5oqK+qhJUwz1JEZScIiS4MLjjl3wUP56f0f3ew Vv1EW/lbUqjKPhtyhjhPaf1g6ww7U/98YxvT17ajFLqT/A0ffyqrH4osjnRFe6ItfFV6 Qafg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to; bh=14icftTA0jvXa4Tee8+dRHyqAezOrcis2S0LKdxY/6o=; b=OvVsYzdk6y53tZRqxay4nDuOzH2u8xX7/6WwJS0jYtXd4IRFt1HVdKmeAP9LyWCCC8 E5yM3Uzd1Zb0jGiGe8XeOTfKGW52YU4/aJPvx7S0ljWoF8191atFlNUn+GbPwSDm1PqN hOvzqSC0e+fAAGKocaR9EP0JVkkqLR+icmpQqmwu3TacT7ZvKO2RB3bKcDS4JN+MFoj9 3NxYlrAwCqEwtZUk3pIk7Z6hGyABuzFcRTr7ofxR/Fpx8wizic0BY0KyxmpKGtZeVvJB QiUoo+btUd/v3Ia2BadGVYFNr3PCjecuEHdvSlzFzdDYMc2Ud7DfNuShLy2syHDw13Ns D7AQ== X-Gm-Message-State: AIkVDXKVTIFTtF/WxbZtrQjiNo56PgWW5xKDH0bTwmcXvjpV+m96/KdIDJ8iFJV1dhFMNBnz X-Received: by 10.223.136.109 with SMTP id e42mr14838414wre.14.1486481459804; Tue, 07 Feb 2017 07:30:59 -0800 (PST) Received: from [10.10.1.58] ([185.97.61.26]) by smtp.gmail.com with ESMTPSA id q12sm19388238wmd.8.2017.02.07.07.30.58 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 07 Feb 2017 07:30:59 -0800 (PST) Subject: Re: svn commit: r313260 - head/sys/kern To: Mateusz Guzik , Alexey Dokuchaev References: <201702050140.v151eRXX090326@repo.freebsd.org> <42c790bb-df12-2a50-6181-24bac5c72d34@multiplay.co.uk> <20170205030006.GB4375@dft-labs.eu> <20170205151746.GA6841@FreeBSD.org> <20170207145750.GD4772@dft-labs.eu> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik From: Steven Hartland Message-ID: <8e0f5a4c-16ec-842d-f4f7-32c830f43553@multiplay.co.uk> Date: Tue, 7 Feb 2017 15:30:58 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170207145750.GD4772@dft-labs.eu> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.23 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Feb 2017 15:31:02 -0000 On 07/02/2017 14:57, Mateusz Guzik wrote: > On Sun, Feb 05, 2017 at 03:17:46PM +0000, Alexey Dokuchaev wrote: >> On Sun, Feb 05, 2017 at 04:00:06AM +0100, Mateusz Guzik wrote: >>> For instance, plugging an unused variable, a memory leak, doing a >>> lockless check first etc. are all pretty standard and unless there is >>> something unusual going on (e.g. complicated circumstances leading to a >>> leak) there is not much to explain. In particular, I don't see why >>> anyone would explain why leaks are bad on each commit plugging one. >> Right; these (unused variable, resource leaks) usually do not warrant >> elaborate explanation. >> >> [ Some linefeeds below were trimmed for brevity ] >>> The gist is as follows: there are plenty of cases where the kernel wants >>> to atomically replace the value of a particular variable. Sometimes, >>> like in this commit, we want to bump the counter by 1, but only if the >>> current value is not 0. For that we need to read the value, see if it is >>> 0 and if not, try to replace what we read with what we read + 1. We >>> cannot just increment as the value could have changed to 0 in the >>> meantime. >>> But this also means that multiple cpus doing the same operation on the >>> same variable will trip on each other - one will succeed while the rest >>> will have to retry. >>> Prior to this commit, each retry attempt would explicitly re-read the >>> value. This induces cache coherency traffic slowing everyone down. >>> amd64 has the nice property of giving us the value it found eleminating >>> the need to explicitly re-read it. There is similar story on i386 and >>> sparc. >>> Other architectures may also benefit from this, but that I did not >>> benchmark. >>> >>> In short[,] under contention atomic_fcmpset is going to be faster than >>> atomic_cmpset. >>> I did not benchmark this particular change, but a switch of the sort >>> easily gives 10%+ in microbenchmarks on amd64. >>> That said, while one can argue this optimizes the code, it really >>> depessimizes it as something of the sort should have been already >>> employed. >> Given the above, IMHO it's quite far from an obvious or of manpage-lookup >> thing, and thus requires proper explanation in the commit log. >> > If the aformenteiond explanation is necessary, the place for it is in > the man page. There are already several commits with fcmpset and there > will be more to come. I don't see why any of them would convey the > information. > The details of why performance under contention of atomic_fcmpset is better than atomic_cmpset, a manpage would be nice. All I'm suggesting is, while one could guess this may be a performance or possibly a compatibility thing, the reason is not obvious, so a small piece of detail on why the change was done should always be included. For this one something like the following would be nice: Switch fget_unlocked to atomic_fcmpset Improve performance under contention by switching fget_unlocked to use atomic_fcmpset. With small piece of additional information, its clear the reason for the change (why) was to improve performance and anyone who wants more detail on why this would be the case can research it via a manpage or other resources, wouldn't you agree? Regards Steve