Re: [PATCH v4 sched_ext/for-6.11 2/2] sched_ext: Implement DSQ iterator

[Tejun Heo <tj@kernel.org>]

Hello, Alexei.

On Mon, Jul 08, 2024 at 03:41:48PM -0700, Alexei Starovoitov wrote:
> In the future pls resubmit the whole series as v4
> (all patches not just one).
> It was difficult for me to find the patch 1/2 without any vN tag
> that corresponds to this v4 patch.
> lore helped at the end.

Sorry about that. That's me being lazy. It looks like even `b4 am` can't
figure out this threading.

> > @@ -1415,7 +1487,7 @@ static void dispatch_enqueue(struct scx_
> >                  * tested easily when adding the first task.
> >                  */
> >                 if (unlikely(RB_EMPTY_ROOT(&dsq->priq) &&
> > -                            !list_empty(&dsq->list)))
> > +                            nldsq_next_task(dsq, NULL, false)))
> 
> There is also consume_dispatch_q() that is doing
> list_empty(&dsq->list) check.
> Does it need to be updated as well?

The one in consume_dispatch_q() is an opportunistic unlocked test as by the
time consume_dispatch_q() is called list head update should be visible
without locking. The test should fail if there's anythingn on the list and
then the code locks the dsq and does proper nldsq_for_each_task(). So, yeah,
that should be a naked list_empty() test. I'll add a comment explaining
what's going on there.

...
> > @@ -6118,6 +6298,9 @@ BTF_KFUNCS_START(scx_kfunc_ids_any)
> >  BTF_ID_FLAGS(func, scx_bpf_kick_cpu)
> >  BTF_ID_FLAGS(func, scx_bpf_dsq_nr_queued)
> >  BTF_ID_FLAGS(func, scx_bpf_destroy_dsq)
> > +BTF_ID_FLAGS(func, bpf_iter_scx_dsq_new, KF_ITER_NEW | KF_RCU_PROTECTED)
> > +BTF_ID_FLAGS(func, bpf_iter_scx_dsq_next, KF_ITER_NEXT | KF_RET_NULL)
> > +BTF_ID_FLAGS(func, bpf_iter_scx_dsq_destroy, KF_ITER_DESTROY)
> >  BTF_ID_FLAGS(func, scx_bpf_exit_bstr, KF_TRUSTED_ARGS)
> >  BTF_ID_FLAGS(func, scx_bpf_error_bstr, KF_TRUSTED_ARGS)
> >  BTF_ID_FLAGS(func, scx_bpf_dump_bstr, KF_TRUSTED_ARGS)
> > --- a/tools/sched_ext/include/scx/common.bpf.h
> > +++ b/tools/sched_ext/include/scx/common.bpf.h
> > @@ -39,6 +39,9 @@ u32 scx_bpf_reenqueue_local(void) __ksym
> >  void scx_bpf_kick_cpu(s32 cpu, u64 flags) __ksym;
> >  s32 scx_bpf_dsq_nr_queued(u64 dsq_id) __ksym;
> >  void scx_bpf_destroy_dsq(u64 dsq_id) __ksym;
> > +int bpf_iter_scx_dsq_new(struct bpf_iter_scx_dsq *it, u64 dsq_id, u64 flags) __ksym __weak;
> > +struct task_struct *bpf_iter_scx_dsq_next(struct bpf_iter_scx_dsq *it) __ksym __weak;
> > +void bpf_iter_scx_dsq_destroy(struct bpf_iter_scx_dsq *it) __ksym __weak;
> >  void scx_bpf_exit_bstr(s64 exit_code, char *fmt, unsigned long long *data, u32 data__sz) __ksym __weak;
> >  void scx_bpf_error_bstr(char *fmt, unsigned long long *data, u32 data_len) __ksym;
> >  void scx_bpf_dump_bstr(char *fmt, unsigned long long *data, u32 data_len) __ksym __weak;
> > --- a/tools/sched_ext/scx_qmap.bpf.c
> > +++ b/tools/sched_ext/scx_qmap.bpf.c
> 
> We typically split kernel changes vs bpf prog and selftests changes
> into separate patches.

Let me think about that. I kinda like putting them into the same patch as
long as they're small as it makes the patch more self-contained but yeah
separating out does have its benefits (e.g. for backporting).

> > +"  -P            Print out DSQ content to trace_pipe every second, use with -b\n"
> 
> tbh the demo of the iterator is so-so. Could have done something more
> interesting :)

Yeah, it's difficult to do something actually interesting with scx_qmap.
Once the scx_bpf_consume_task() part lands, the example can become more
interesting. scx_lavd is already using the iterator. Its usage is a lot more
interesting and actually useful (note that the syntax is a bit different
right now, will be synced soon):

  https://github.com/sched-ext/scx/blob/main/scheds/rust/scx_lavd/src/bpf/main.bpf.c#L2041

Thanks.

-- 
tejun