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

[Alexei Starovoitov]

On Mon, Jul 8, 2024 at 4:40 PM Tejun Heo <tj@xxxxxxxxxx> wrote:
>
>
> > > @@ -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.

I see. Thanks for adding a comment.

> > > --- 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).

We split kernel vs libbpf vs selftest patches, because libbpf patches
get synced into github and it's released from there, while
kernel patches get backported, and selftests don't have to be backported.

> > > +"  -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 for the extra context.