Re: [PATCH sched_ext/for-6.15 v3 2/5] sched_ext: Declare context-sensitive kfunc groups that can be used by different SCX operations

[Juntong Deng]

On 2025/2/28 21:57, Tejun Heo wrote:

> On Fri, Feb 28, 2025 at 06:42:11PM +0000, Juntong Deng wrote:
> > > > Return 0 means allowed. So kfuncs in scx_kfunc_ids_unlocked can be
> > > > called by other struct_ops programs.
> >
> > > Hmm... would that mean a non-sched_ext bpf prog would be able to call e.g.
> > > scx_bpf_dsq_insert()?
> >
> > For other struct_ops programs, yes, in the current logic,
> > when prog->aux->st_ops != &bpf_sched_ext_ops, all calls are allowed.
> >
> > This may seem a bit weird, but the reason I did it is that in other
> > struct_ops programs, the meaning of member_off changes, so the logic
> > that follows makes no sense at all.
> >
> > Of course, we can change this, and ideally there would be some groupings
> > (kfunc id set) that declare which kfunc can be called by other
> > struct_ops programs and which cannot.

> Other than any and unlocked, I don't think other bpf struct ops should be
> able to call SCX kfuncs. They all assume rq lock to be held which wouldn't
> be true for other struct_ops after all.

Ok, I will allow only any and unlocked to be called by other struct_ops
programs in the next version.

> ...
> > > I see, scx_dsq_move_*() are in both groups, so it should be fine. I'm not
> > > fully sure the groupings are the actually implemented filtering are in sync.
> > > They are intended to be but the grouping didn't really matter in the
> > > previous implementation. So, they need to be carefully audited.
> >
> > After you audit the current groupings of scx kfuncs, please tell me how
> > you would like to change the current groupings.

> Yeah, I'll go over them but after all, we need to ensure that the behavior
> currently implemented by scx_kf_allowed*() matches what the new code does,
> so I'd appreciate if you can go over with that in mind too. This is kinda
> confusing so we can definitely use more eyes.

Yes, I will use more eyes and be more careful on consistency.

> On Wed, Feb 26, 2025 at 07:28:17PM +0000, Juntong Deng wrote:
> > This patch declare context-sensitive kfunc groups that can be used by
> > different SCX operations.
> >
> > In SCX, some kfuncs are context-sensitive and can only be used in
> > specific SCX operations.
> >
> > Currently context-sensitive kfuncs can be grouped into UNLOCKED,
> > CPU_RELEASE, DISPATCH, ENQUEUE, SELECT_CPU.
> >
> > In this patch enum scx_ops_kf_flags was added to represent these groups,
> > which is based on scx_kf_mask.
> >
> > SCX_OPS_KF_ANY is a special value that indicates kfuncs can be used in
> > any context.
> >
> > scx_ops_context_flags is used to declare the groups of kfuncs that can
> > be used by each SCX operation. An SCX operation can use multiple groups
> > of kfuncs.
>
> Can you merge this into the next patch? I don't think separating this out
> helps with reviewing.

Yes, I can merge them in the next version.

I am not sure, but it seems to me that the two patches are doing
different things?

> Thanks.