From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753519AbbG0M7X (ORCPT ); Mon, 27 Jul 2015 08:59:23 -0400 Received: from mail-wi0-f181.google.com ([209.85.212.181]:38085 "EHLO mail-wi0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752235AbbG0M7W (ORCPT ); Mon, 27 Jul 2015 08:59:22 -0400 Subject: Re: [PATCH] userns: simplify map_id_range_* functions To: "Eric W. Biederman" References: <1437875432-15502-1-git-send-email-nicolas.iooss_linux@m4x.org> <878ua26x5v.fsf@x220.int.ebiederm.org> Cc: Andy Lutomirski , Andrew Morton , linux-kernel@vger.kernel.org From: Nicolas Iooss X-Enigmail-Draft-Status: N1110 Message-ID: <55B62B23.8050008@m4x.org> Date: Mon, 27 Jul 2015 20:59:15 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <878ua26x5v.fsf@x220.int.ebiederm.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/27/2015 12:29 PM, Eric W. Biederman wrote: > Nicolas Iooss writes: > >> Functions map_id_range_down, map_id_down and map_id_up all used the >> construction: >> >> if (...) >> id = ... >> else >> id = ... >> return id; >> >> which can be simplified by directly returning the result of the >> computations in each branch. >> >> Moreover as the condition tested whether the "break;" in the previous >> for loop was hit, it is simpler to directly compute the result and >> return it. > > It is not a simplification, it is just code motion. I agree. Also on my system (x86_64 kernel with Arch Linux + CONFIG_USERNS configuration), the assembly code generated either by gcc 5.2.0 or by clang 3.6.2 (with LLVMLinux patches) does not change at all with this patch. So there would be absolutely no performance change or similar things which could motivate this change. > Further at least to my eyes adding multiple exit points and setting the > same value in two different places actually obscures what the functions > are doing. I did not understand what "setting the same value in two different places" refers to. Anyway, all right. I haven't got much experience about code maintenance so if you say my patch make things less clear, I believe you. > If we could talk about speeding up the performance of the stat system > call I think there would be a point in mucking with these functions. > > As it is I think it is I think merging your patch will just make it more > difficult to understand what the code is doing in the future, with no > benefit except a reduction in line count. OK. My intention was precisely to make the code easier to understand (because I spent some time before I understood there really were only two cases: existing mapping or not) but if you think my patch does the opposite, I accept dropping it. Thanks for your comments. Nicolas