Received: by 2002:a05:6602:2086:0:0:0:0 with SMTP id a6csp4382073ioa; Wed, 27 Apr 2022 02:32:30 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxRSWtAFYTGLGrFnhO3HE/YfDXQ+z/gz1iiyuadQRxYQrMMUDiIhVOFpBWGbp7DWKENoAVw X-Received: by 2002:a17:903:2d0:b0:14d:8a8d:cb1 with SMTP id s16-20020a17090302d000b0014d8a8d0cb1mr27952063plk.50.1651051949815; Wed, 27 Apr 2022 02:32:29 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1651051949; cv=none; d=google.com; s=arc-20160816; b=sZcXZaK/75S77wNtsABLviMJBTLDGa/KeTT6GhFKe8aq6QZnVxPEVK9bkpgznvzL0l 2PtZU791MeQ0rJyJl5dEpCJTRy28k9u1N6R1LKFZYGEeWkpipfpCsHOTZQHo7yOldU48 DgDASbTDJ+G1rcoNWap5WHsa9txZd6/YHrYYQnzpwFoX6Ot4bmrxETsG702/fYuH7/RJ zh+WYrA87zRnB9XZfAywibKqqc5CGAr5hpdlUSiA9ZA4rEN+L/1EQiBEJPEJF4537CVG 0OBKC4Il8YaukKh0dYaAyegPh/xtd/mPQ4OxR7/775k5koq7mE14mt2HW5l9Ih0HLwsa sV+g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date; bh=4AJa6o9cnp86UMGSkFzzEBQFQunBQTis642GseNmaDc=; b=aHhHvqoYOBeGq6hrrhoghBx6qsravchGoFBpWhVRLioE+qcyFokkeLF5OYh4DoOUx0 jvMJLfcg1Q2WE8I4vP9QJOoV6Wap3kYvg6DBTgwzYJxOmdFZWG8z6UIih3rpRzKTQNh7 9pOPr9/6gnTvagpRFgjz/lRrMEe6dYzNkDZVHtGyLNV8TbVDcwd+0DzT+NPWB5u6EHKr 1WYZgm22BvKEmHBfaR78mAD0YWmJY3goC/0sTOt4/pYjLlKs17v+HHV84DKfu0WcrWMF ZOpYEXbKe1h0J8w87Gumv4kE1pEHHRQgxAwUlh/ECCC6aRMojaXGRW5xXCfauZq8yMrN HUFw== ARC-Authentication-Results: i=1; mx.google.com; spf=softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Return-Path: Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net. [23.128.96.19]) by mx.google.com with ESMTPS id d10-20020a170902854a00b00154c1348e40si957083plo.497.2022.04.27.02.32.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 27 Apr 2022 02:32:29 -0700 (PDT) Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19; Authentication-Results: mx.google.com; spf=softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id E6597985B0; Wed, 27 Apr 2022 02:12:57 -0700 (PDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1347454AbiDZONR (ORCPT + 99 others); Tue, 26 Apr 2022 10:13:17 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53974 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1351446AbiDZONP (ORCPT ); Tue, 26 Apr 2022 10:13:15 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 797C9F08 for ; Tue, 26 Apr 2022 07:10:05 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 025D323A; Tue, 26 Apr 2022 07:10:05 -0700 (PDT) Received: from wubuntu (unknown [10.57.77.60]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 794CB3F73B; Tue, 26 Apr 2022 07:10:01 -0700 (PDT) Date: Tue, 26 Apr 2022 15:09:59 +0100 From: Qais Yousef To: Peter Zijlstra Cc: Alexei Starovoitov , Delyan Kratunov , Namhyung Kim , Arnaldo Carvalho de Melo , "bigeasy@linutronix.de" , "dietmar.eggemann@arm.com" , "keescook@chromium.org" , "x86@kernel.org" , "andrii@kernel.org" , "u.kleine-koenig@pengutronix.de" , "vincent.guittot@linaro.org" , "akpm@linux-foundation.org" , "mingo@kernel.org" , "linux-kernel@vger.kernel.org" , "rdunlap@infradead.org" , "rostedt@goodmis.org" , "Kenta.Tada@sony.com" , "tglx@linutronix.de" , "bristot@redhat.com" , "ebiederm@xmission.com" , "ast@kernel.org" , "legion@kernel.org" , "adharmap@quicinc.com" , "valentin.schneider@arm.com" , "ed.tsai@mediatek.com" , "juri.lelli@redhat.com" Subject: Re: [PATCH] sched/tracing: append prev_state to tp args instead Message-ID: <20220426140959.op6u5m7id57aq7yc@wubuntu> References: <20220120162520.570782-1-valentin.schneider@arm.com> <93a20759600c05b6d9e4359a1517c88e06b44834.camel@fb.com> <20220422110903.GW2731@worktop.programming.kicks-ass.net> <056e9bb0d0e3fc20572d42db7386face1d0665d6.camel@fb.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RDNS_NONE, SPF_HELO_NONE autolearn=no 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: X-Mailing-List: linux-kernel@vger.kernel.org On 04/26/22 14:28, Peter Zijlstra wrote: > On Fri, Apr 22, 2022 at 11:30:12AM -0700, Alexei Starovoitov wrote: > > On Fri, Apr 22, 2022 at 10:22 AM Delyan Kratunov wrote: > > > > > > On Fri, 2022-04-22 at 13:09 +0200, Peter Zijlstra wrote: > > > > And on the other hand; those users need to be fixed anyway, right? > > > > Accessing prev->__state is equally broken. > > > > > > The users that access prev->__state would most likely have to be fixed, for sure. > > > > > > However, not all users access prev->__state. `offcputime` for example just takes a > > > stack trace and associates it with the switched out task. This kind of user > > > would continue working with the proposed patch. > > > > > > > If bpf wants to ride on them, it needs to suffer the pain of doing so. > > > > > > Sure, I'm just advocating for a fairly trivial patch to avoid some of the suffering, > > > hopefully without being a burden to development. If that's not the case, then it's a > > > clear no-go. > > > > > > Namhyung just sent this patch set: > > https://patchwork.kernel.org/project/netdevbpf/patch/20220422053401.208207-3-namhyung@kernel.org/ > > That has: > > + * recently task_struct->state renamed to __state so it made an incompatible > + * change. > > git tells me: > > 2f064a59a11f ("sched: Change task_struct::state") > > is almost a year old by now. That don't qualify as recently in my book. > That says that 'old kernels used to call this...'. > > > to add off-cpu profiling to perf. > > It also hooks into sched_switch tracepoint. > > Notice it deals with state->__state rename just fine. > > So I don't speak BPF much; it always takes me more time to make bpf work > than to just hack up the kernel, which makes it hard to get motivated. > > However, it was not just a rename, state changed type too, which is why I > did the rename, to make sure all users would get a compile fail and > could adjust. > > If you're silently making it work by frobbing the name, you loose that. > > Specifically, task_struct::state used to be 'volatile long', while > task_struct::__state is 'unsigned int'. As such, any user must now be > very careful to use READ_ONCE(). I don't see that happening with just > frobbing the name. > > Additinoally, by shrinking the field, I suppose BE systems get to keep > the pieces? > > > But it will have a hard time without this patch > > until we add all the extra CO-RE features to detect > > and automatically adjust bpf progs when tracepoint > > arguments order changed. > > Could be me, but silently making it work sounds like fail :/ There's a > reason code changes, users need to adapt, not silently pretend stuff is > as before. > > How will you know you need to fix your tool? If libbpf doesn't fail, then yeah it's a big problem. I wonder how users of kprobe who I suppose are more prone to this kind of problems have been coping. > > > We will do it eventually, of course. > > There will be additional work in llvm, libbpf, kernel, etc. > > But for now I think it would be good to land Delyan's patch > > to avoid unnecessary pain to all the users. > > > > Peter, do you mind? > > I suppose I can help out this time, but I really don't want to set a > precedent for these things. Broken is broken. > > The down-side for me is that the argument order no longer makes any > sense. I'm intending to backport fa2c3254d7cf to 5.10 and 5.15 but waiting for a Tested-by. If you take this one, then it'll need to be backported too. Cheers -- Qais Yousef