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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C5CFAECAAA6 for ; Fri, 26 Aug 2022 16:42:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344308AbiHZQmO (ORCPT ); Fri, 26 Aug 2022 12:42:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60120 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1343791AbiHZQl7 (ORCPT ); Fri, 26 Aug 2022 12:41:59 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7990C3AB01; Fri, 26 Aug 2022 09:41:55 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 302A4B831D7; Fri, 26 Aug 2022 16:41:54 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 548EEC43141; Fri, 26 Aug 2022 16:41:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1661532112; bh=XeDtcz1Gd48tZyQA1aYnSkeMEWO+Xd1Bf+UvW5Grxqk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=PvRKmfKFGihJsHTX6bGgDE8r/+C9WObW+Q2c8U7mDZ5zT/VHDBQN4CCtyymREWkve wsP+ilQ0QfksG81m+A1t/cH0/B3xRU414ceOGxSLJgla5dF5Ae1UhC3iijxIk1uCkx hjqkiVhSFt7IfVwqszEYGkYqvbFI28fujs1qXv2hMVpDO+ooSM+CDr3Nclx0HjVsgZ J79Tl2mH3595+9dyrfzAguoKXxlLauzaMw/yVB4Lf6K/zEvw24v0EvGiUfUf1B2Qxk K67dUMWdkyPDTOG264CgGX4rS+4CsFG34E3EtTpEwieJx6Q1oc/t/vwPAqcuVkb3mW n7XhNOqyT8xhw== Date: Fri, 26 Aug 2022 19:41:45 +0300 From: Jarkko Sakkinen To: Roberto Sassu Cc: Alexei Starovoitov , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Martin KaFai Lau , Song Liu , Yonghong Song , John Fastabend , KP Singh , Stanislav Fomichev , Hao Luo , Jiri Olsa , Mykola Lysenko , Jonathan Corbet , David Howells , Steven Rostedt , Ingo Molnar , Paul Moore , James Morris , "Serge E . Hallyn" , Shuah Khan , bpf , "open list:DOCUMENTATION" , keyrings@vger.kernel.org, LSM List , "open list:KERNEL SELFTEST FRAMEWORK" , LKML , Daniel =?iso-8859-1?Q?M=FCller?= , Roberto Sassu , Joanne Koong Subject: Re: [PATCH v12 02/10] btf: Handle dynamic pointer parameter in kfuncs Message-ID: References: <20220818152929.402605-1-roberto.sassu@huaweicloud.com> <20220818152929.402605-3-roberto.sassu@huaweicloud.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Aug 26, 2022 at 07:32:54PM +0300, Jarkko Sakkinen wrote: > On Fri, Aug 26, 2022 at 05:34:57PM +0200, Roberto Sassu wrote: > > On Fri, 2022-08-26 at 17:43 +0300, Jarkko Sakkinen wrote: > > > On Fri, Aug 26, 2022 at 08:46:14AM +0300, Jarkko Sakkinen wrote: > > > > On Thu, Aug 25, 2022 at 10:16:14PM -0700, Alexei Starovoitov wrote: > > > > > On Thu, Aug 25, 2022 at 9:54 PM Jarkko Sakkinen < > > > > > jarkko@kernel.org> wrote: > > > > > > > -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env > > > > > > > *env, struct bpf_reg_state *reg, > > > > > > > - enum bpf_arg_type > > > > > > > arg_type) > > > > > > > +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, > > > > > > > struct bpf_reg_state *reg, > > > > > > > + enum bpf_arg_type arg_type) > > > > > > > { > > > > > > > struct bpf_func_state *state = func(env, reg); > > > > > > > int spi = get_spi(reg->off); > > > > > > > -- > > > > > > > 2.25.1 > > > > > > > > > > > > > > > > > > > Might be niticking but generally I'd consider splitting > > > > > > exports as commits of their own. > > > > > > > > > > -static bool > > > > > +bool > > > > > > > > > > into a separate commit? > > > > > > > > > > I guess it makes sense for people whose salary depends on > > > > > number of commits. > > > > > We don't play these games. > > > > > > > > What kind of argument is that anyway. > > > > > > "Separate each *logical change* into a separate patch." [*] > > > > The logical change, as per the patch subject, is allowing the > > possibility of including eBPF dynamic pointers in a kfunc definition. > > It requires to call an existing function that was already defined > > elsewhere. > > > > Maybe I'm wrong, but I don't see only exporting a function definition > > to an include file as a logical change. To me, the changes in this > > patch are clearly connected. Or even better, they tell why the function > > definition has been exported, that would not appear if moving the > > function definition is a standalone patch. > > > > > > > > To add, generally any user space visible space should be an > > > isolated patch. > > > > As far as I understood, definitions visible to user space should be in > > include/uapi. > > It does change e.g. the output of kallsyms. > > It's not ABI but it's still user space visble. > > > > > > > > > Please, stop posting nonsense. > > > > If I may, saying this does not encourage people to try to submit their > > code. I feel it is a bit strong, and I kindly ask you to express your > > opinion in a more gentle way. > > I agree. That's why I was wondering what is this nonsense > about salary and games. Please denote that I started my review with "Might be nitpicking...". It's neither particularly disencouraging nor enforcing for anyone. The blast that came after that, on the other hand, IMHO meets exactly those standards. BR, Jarkko