From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3E3DE189F3F for ; Fri, 17 Jan 2025 16:24:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737131087; cv=none; b=agAS8RszhpR65zZRMEhehzWo4cQi33Sw9IM/WPZqHDybOih5SljUwVvuliQNwU5DwOsYW/ECJik5JN+qwHxwYP0WbisJMDq+fuVa5y173U5cD5kL0HvCj3KOFx21f6Urb3DpCp4L8cy/LbRASX+Pv9bflMLXvOzwIA5VZOhnEWE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737131087; c=relaxed/simple; bh=B+NOO8M/CqhdNVGeFTMrr7zwbsoHEr4J43KsnhNgKtU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=YQzlrv8zXVFAjYCF9ZLmMNdXdDpeBAiwr0FZMicHa8/yA/iyTAL/yDh1Q0Zgb6uGcYIY2FpdGrKOmcXK/D84JhfJY4JKp+2zWgmCD50OX2MNacJF6GpTdTU8o3p/iGroS32o+1HmZT2O+FiOxOeHZACH6WwnsgQC+uOG7Mmb1jo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=EHzjGTIC; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="EHzjGTIC" Received: by smtp.kernel.org (Postfix) with ESMTPSA id A2F98C4CEDD; Fri, 17 Jan 2025 16:24:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1737131086; bh=B+NOO8M/CqhdNVGeFTMrr7zwbsoHEr4J43KsnhNgKtU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=EHzjGTICcKYuJD6jmjvNQ8mKHfvm/mjVWqY6fJ5O1IXt2d0lfhTj59Nx8zbJDz7GO z3gBMMKLQEBPghc72zzq7sq+bNKUda7foh0A2vdAzvWsG43Uj7NS49SI+sudu3WqcR KvoRB6cbebvYIyOfXbhGHvGYaYWRe7H6jJiPQHNXKI8Al8LcW8w/WA2m7ACYYeHOAo Si6hvIlzoT1USRxjHAVDzuGcZ4AoO1owaT1xh2+wqv+2KvSATsLkHd36wnNmJFZ8vv T88MHEQDVdU/Nt0iR4JlbBld+Z5j0jmYoBzxk9ZuEdGGiBEKLxh0rBpJBWccxygf+F 2jjupn2pDY++Q== Date: Fri, 17 Jan 2025 06:24:45 -1000 From: Tejun Heo To: Changwoo Min Cc: void@manifault.com, arighi@nvidia.com, kernel-dev@igalia.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/7] sched_ext: Implement event counter infrastructure and add an event Message-ID: References: <20250116151543.80163-1-changwoo@igalia.com> <20250116151543.80163-2-changwoo@igalia.com> <16a9e631-22c0-454a-b56f-c5206ec3d1dc@igalia.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <16a9e631-22c0-454a-b56f-c5206ec3d1dc@igalia.com> Hello, On Fri, Jan 17, 2025 at 04:08:45PM +0900, Changwoo Min wrote: > $COMPONENT_$EVENT sounds more systematic. Thanks for the > suggestion. What about SELECT_CPU_FALLBACK? Sounds good. > > This feels a bit odd to me. Why does it need both the struct fields and > > indices? Can we do either one of those? > > Before submitting the patch set, I tested one approach using > purely enums storing the actual counters in an array and another > (current) approach using struct fields. I rejected the first > approach because the BPF verifier does not allow changing the > array size, causing the BPF binary compatibility problem. The That makes sense. It'd be useful to note that on top of the struct definition. > second approach is satisfactory regarding the BPF binary > compatibility by CO-RE. However, it is a little bit cumbersome to > iterate all the events, so I added the enums. I know the enums > are only usable in the kernel code due to the binary > compatibility, but I thought adding the enums rather than > bloating the scx_dump_state() code at the end would be better. > Does it make sense to you? Let's just use the structs and open code dumping. We can pack the dump output better that way too and I don't think BPF scheds would have much use for enum iterations. ... > > > +/** > > > + * scx_add_event - Increase an event counter for 'name' by 'cnt' > > > + * @name: an event name defined in struct scx_event_stat > > > + * @cnt: the number of the event occured > > > + */ > > > +#define scx_add_event(name, cnt) ({ \ > > > + struct scx_event_stat *__e; \ > > > + __e = get_cpu_ptr(&event_stats); \ > > > + WRITE_ONCE(__e->name, __e->name+ (cnt)); \ > > > + put_cpu_ptr(&event_stats); \ > > > > this_cpu_add()? > > That's handier. I will change it. Note that there's __this_cpu_add() too which can be used from sections of code that already disables preemption. On x86, both variants cost the same but on arm __this_cpu_add() is cheaper as it can skip preemption off/on. Given that most of these counters are modified while holding rq lock, it may make sense to provide __ version too. ... > > Also, I'm not sure this is a useful event to count. False ops_cpu_valid() > > indicates that the returned CPU is not even possible and the scheduler is > > ejected right away. What's more interesting is > > kernel/sched/core.c::select_task_rq() tripping on !is_cpu_allowed() and > > falling back using select_fallback_rq(). > > > > We can either hook into core.c or just compare the ops.select_cpu() picked > > CPU against the CPU the task ends up on in enqueue_task_scx(). > > Modifying core.c will be more direct and straightforward. Also, > I think it would be better to separate this commit into two: one > for the infra-structure and another for the SELECT_CPU_FALLBACK > event, which touches core.c. I will move the necessary code in > the infrastructure into kernel/sched/sched.h, so we can use > scx_add_event() in core.c. Hmm.... yeah, both approaches have pros and cons but I kinda like the idea of restricting the events within ext.c and here detecting the fallback condition is pretty trivial. I don't know. Thanks. -- tejun