Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Song Liu <songliubraving@fb.com>
To:     Steven Rostedt <rostedt@goodmis.org>
CC:     Petr Mladek <pmladek@suse.com>, kernel test robot <lkp@intel.com>,
        Song Liu <song@kernel.org>, bpf <bpf@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        "live-patching@vger.kernel.org" <live-patching@vger.kernel.org>,
        "kbuild-all@lists.01.org" <kbuild-all@lists.01.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Kernel Team <Kernel-team@fb.com>,
        "jolsa@kernel.org" <jolsa@kernel.org>
Subject: Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on
 the same function
Thread-Topic: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on
 the same function
Thread-Index: AQHYmmrwWVTrDYAzL0G5ckglE1ecDq2DvwKAgABeKwCAAD2SAIABrCEAgABCBgA=
Date:   Tue, 19 Jul 2022 22:28:30 +0000
Message-ID: <8A0701E5-E110-4DAD-9560-88FF87214286@fb.com>
References: <20220718055449.3960512-3-song@kernel.org>
 <202207181552.VuKfz9zg-lkp@intel.com> <YtVd4FKOcEmGfubm@alley>
 <9DAB0710-7D60-46AC-8A2F-ED4B8A1A4BC0@fb.com>
 <20220719143210.08f9922b@gandalf.local.home>
In-Reply-To: <20220719143210.08f9922b@gandalf.local.home>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?q62fPjwRWItS76Q8H45IrNRwbsSSIPctvJjacxf/4ERloIwqo/Tv68MDEhCn?=
 =?us-ascii?Q?rUaCszkda5jBw9hmSxMuEGn6KDDvfKN0gndVBQOKiO20o4oAGrn/GW7frp//?=
 =?us-ascii?Q?/llXdhDBquZrSsV7olIAvlfcOnMjnZwHQBbuAQGKZYelbqdhP8qXuv5zOB2B?=
 =?us-ascii?Q?3ZhIF7HP3fi7pfLILa6hnI1pu9a6ok0WkUKCnlEzJBy1tu3igNbS+V1N2XGd?=
 =?us-ascii?Q?+JPigLkgx+1yWFhXJPsb2iipOLMBI93XCvLSpwfuGTKme5AnFzIyFva+s0pd?=
 =?us-ascii?Q?R+cNEU3rJYdYXyb2hUHN1t4pW7OUbcmVr6B1z9VqzU+ElaaBEQpTkEtbZWc9?=
 =?us-ascii?Q?sOZ3FnKEBCe76mfMKkMjXLXmLKuJte196v8cgqD4cL/zFQt0Iihnutyix5dO?=
 =?us-ascii?Q?HtKq4L8vVW0ywudfYJ+PLFwYyHiivW3TyjEFZjBMYb/nmrDSnIH033SbTfaM?=
 =?us-ascii?Q?HAJSx7WInMdjuAc8TjfE/4SgVeaD0ZFlpxvXqXvmoV/d+umkyJ7EXko9Xwm6?=
 =?us-ascii?Q?aIJ8C/kITzxZTe3HZ6nF4KwAQluy9zlkviwdhW1dynpidPVJQXUs/n4ev+g/?=
 =?us-ascii?Q?FSCfZpGcc2PNVkXHDWj/FO6LoKKOsPZMIam9tPWNmrDsHvAF1bbXODg5X2nr?=
 =?us-ascii?Q?jpU9+LB7gCt4LqGamQNwTbfNqB6B00LtDSeF877PRqOyk+ACe58h/zMHzy3/?=
 =?us-ascii?Q?4AHcjTuicP0MQKy3ldSxfeNgQjxbcFWVTdeeYrgqSHzM0Kid3c4j2aeJKg3J?=
 =?us-ascii?Q?7pKfe+YXMDe/T5fUibahbzYLDgSvHa1953u5rghdK+1JSVJrigo2C3e38Z4a?=
 =?us-ascii?Q?NQ11ydfjdDbs23hjIK5ykA/bb9KHlfHWQ5X2WaGps7ANncWZwMj/BK4bXfyb?=
 =?us-ascii?Q?wdaS3ulFKfpFcDLkUI3L4YUgXBgJCTF4rNV12kkpSUZXNtyiPoZlyfQ2im3j?=
 =?us-ascii?Q?xvwqFMjxQPUJ+4+eVm89Y8zk/+xiIbkxjovg9ESpt/qV5TzQ71mYi2MC0lSx?=
 =?us-ascii?Q?xI8l8PSr1B7baxWMQ1xoIdBRWM8UxnS4GpwB0PEGtsemBi3t4kVEVDURMHZk?=
 =?us-ascii?Q?lJoTSv2IhmL3OC2Nbp0RcsDsEL5RjhHAkMsLhya5nQCkJcl40OrMIVC/eoMf?=
 =?us-ascii?Q?uODq+1SIOPcJuyIyPpaQL/zMCfDD+zXEhJOAgEa81176UmPKqzvskQHwr0v+?=
 =?us-ascii?Q?HxEzR0ggALdHu+v/ifCyoikq/JW/D4mhxPUF+I0eQJD/V7MUJxiCed8sqFOs?=
 =?us-ascii?Q?m7g13TL4pzeQ1r/0FeRGqz4Po1rIWbdKWKI7GJkANarVBEH+PtqyLlYbbIqy?=
 =?us-ascii?Q?cl9UMZsx7GShbiTRh5IVMARXTbyibUmvqP8Mm+OdeAEW4FPfv6pNQ7xGb+a9?=
 =?us-ascii?Q?JoCkpgG24NnveRYdGwZSOxHRFDrvZ6jKGXqHo7pSzpWBW0oQCGmvU1p/Dvdr?=
 =?us-ascii?Q?FW2N7PjMM+hPptI4y52XFtJhh7AEIm+1HvCvfF29nJsa3EGXxyOpdp1a9WTD?=
 =?us-ascii?Q?1Qj97Xy20MRgqnP2/LH5Uu/Gwh9AV3vLhyAFTcglCdJtL9wJ/ZrAdKlQtB9B?=
 =?us-ascii?Q?TP+OtCPXH7IJmbckZ2G3BSusLHewMRTd8gkkraGA6bOIG0muH1aGhMpghe+A?=
 =?us-ascii?Q?1/qBQVIoX1cApFUW+82zqoI=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <6635582B67FD7443A8D341E41B3F7D55@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3785dc6a-d8f9-41c6-5bc5-08da69d601ed
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Jul 2022 22:28:30.2284
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: MsGGXG90lwGA5BDdq44uhT4lHmZpA4DIARIIOt/Ev42uvK5Fb3vvCfFprRG0L9gXRVcWWJaoAyRR6Ezpz0v7YA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1549
X-Proofpoint-ORIG-GUID: ZgVQf-gbvpAO586glCl6aEkY3v3X90Iy
X-Proofpoint-GUID: ZgVQf-gbvpAO586glCl6aEkY3v3X90Iy
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-07-19_08,2022-07-19_01,2022-06-22_01
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Jul 19, 2022, at 11:32 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Mon, 18 Jul 2022 16:59:51 +0000
> Song Liu <songliubraving@fb.com> wrote:
> 
>>>> vim +/direct_mutex_locked +8197 kernel/trace/ftrace.c
>>>> 
>>>> 8182	
>>>> 8183	/**
>>>> 8184	 * register_ftrace_function - register a function for profiling
>>>> 8185	 * @ops:	ops structure that holds the function for profiling.
>>>> 8186	 *
>>>> 8187	 * Register a function to be called by all functions in the
>>>> 8188	 * kernel.
>>>> 8189	 *
>>>> 8190	 * Note: @ops->func and all the functions it calls must be labeled
>>>> 8191	 *       with "notrace", otherwise it will go into a
>>>> 8192	 *       recursive loop.
>>>> 8193	 */
>>>> 8194	int register_ftrace_function(struct ftrace_ops *ops)
>>>> 8195		__releases(&direct_mutex)
>>>> 8196	{  
>>>>> 8197		bool direct_mutex_locked = false;  
>>>> 8198		int ret;
>>>> 8199	
>>>> 8200		ftrace_ops_init(ops);
>>>> 8201	
>>>> 8202		ret = prepare_direct_functions_for_ipmodify(ops);
>>>> 8203		if (ret < 0)
>>>> 8204			return ret;
>>>> 8205		else if (ret == 1)
>>>> 8206			direct_mutex_locked = true;  
>>> 
>>> Honestly, this is another horrible trick. Would it be possible to
>>> call prepare_direct_functions_for_ipmodify() with direct_mutex
>>> already taken?
> 
> Agreed. I'm not sure why I didn't notice this in the other versions.
> Probably was looking too much at the other logic. :-/
> 
>>> 
>>> I mean something like:
>>> 
>>> 	mutex_lock(&direct_mutex);
>>> 
>>> 	ret = prepare_direct_functions_for_ipmodify(ops);
>>> 	if (ret)
>>> 		goto out:
>>> 
>>> 	mutex_lock(&ftrace_lock);
>>> 	ret = ftrace_startup(ops, 0);
>>> 	mutex_unlock(&ftrace_lock);
>>> 
>>> out:
>>> 	mutex_unlock(&direct_mutex);
>>> 	return ret;  
>> 
>> Yeah, we can actually do something like this. We can also move the
>> ops->flags & FTRACE_OPS_FL_IPMODIFY check to 
>> register_ftrace_function(), so we only lock direct_mutex when when
>> it is necessary. 
> 
> No need. Just take the direct_mutex, and perhaps add a:
> 
> 	lockdep_assert_held(&direct_mutex);
> 
> in the prepare_direct_functions_for_ipmodify().
> 
> This is far from a fast path to do any tricks in trying to optimize it.

Got it. I will fix these and send v5. 

Thanks,
Song