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
