Re: [PATCH v2 bpf-next 2/2] selftests/bpf: Add selftests for cpuidle_gov_ext

["yikai.lin" <yikai.lin@vivo.com>]

On 9/3/2025 8:38 AM, Alexei Starovoitov wrote:
> [Some people who received this message don't often get email from alexei.starovoitov@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Mon, Sep 1, 2025 at 6:56 AM Lin Yikai <yikai.lin@vivo.com> wrote:
>>
>> +
>> +/*
>> + * For some low-power scenarios,
>> + * such as the screen off scenario of mobile devices
>> + * (which will be determined by the user-space BPF program),
>> + * we aim to choose a deeper state
>> + * At this point, we will somewhat disregard the impact on CPU performance.
>> + */
>> +int expect_deeper = 0;
> 
> ...
> 
>> +/* Select the next idle state */
>> +SEC("struct_ops.s/select")
>> +int BPF_PROG(bpf_cpuidle_select, struct cpuidle_driver *drv, struct cpuidle_device *dev)
>> +{
>> +       u32 key = 0;
>> +       s64 delta, latency_req, residency_ns;
>> +       int i;
>> +       unsigned long long disable;
>> +       struct cpuidle_gov_data *data;
>> +       struct cpuidle_state *cs;
>> +
>> +       data = bpf_map_lookup_percpu_elem(&cpuidle_gov_data_map, &key, dev->cpu);
>> +       if (!data) {
>> +               bpf_printk("cpuidle_gov_ext: [%s] cpuidle_gov_data_map is NULL\n", __func__);
>> +               return 0;
>> +       }
>> +       latency_req = bpf_cpuidle_ext_gov_latency_req(dev->cpu);
>> +       delta = bpf_tick_nohz_get_sleep_length();
>> +
>> +       update_predict_duration(data, drv, dev);
>> +       for (i = ARRAY_SIZE(drv->states)-1; i > 0; i--) {
>> +               if (i >= drv->state_count)
>> +                       continue;
>> +               cs = &drv->states[i];
>> +               disable = dev->states_usage[i].disable;
>> +               if (disable)
>> +                       continue;
>> +               if (latency_req < cs->exit_latency_ns)
>> +                       continue;
>> +
>> +               if (delta < cs->target_residency_ns)
>> +                       continue;
>> +
>> +               if (data->next_pred / FIT_FACTOR * ALPHA_SCALE < cs->target_residency_ns)
>> +                       continue;
>> +
>> +               break;
>> +       }
>> +       residency_ns = drv->states[i].target_residency_ns;
>> +       if (expect_deeper &&
>> +               i <= drv->state_count-2 &&
>> +               !dev->states_usage[i+1].disable &&
>> +               data->last_pred >= residency_ns &&
>> +               data->next_pred < residency_ns &&
>> +               data->next_pred / FIT_FACTOR * ALPHA_SCALE >= residency_ns &&
>> +               data->next_pred / FIT_FACTOR * ALPHA_SCALE >= data->last_duration &&
>> +               delta > residency_ns) {
>> +               i++;
>> +       }
>> +
>> +       return i;
>> +}
> 
> This function is the main programmability benefit that
> you're claiming, right?
> 
> And user space knob 'expect_deeper' is the key difference
> vs all existing governors ?
> 
> If so, I have to agree with Rafael. This doesn't look too compelling
> to bolt bpf struct-ops onto cpuidle.
> 
> There must be a way to introduce user togglable knobs in the current
> set of governors, no?
> 
> Other than that the patch set seems to be doing all the right things
> from bpf perspective. KF_SLEEPABLE is missing in kfuncs and
> the safety aspect needs to be thoroughly analyzed,
> but before doing in-depth review the examples need to have more substance.
> With real world benchmarks and results.
> The commit log is saying:
> "This implementation serves as a foundation, not a final solution"
> It's understood that it's work-in-progress, but we need to see
> more real usage before committing.

Thanks, Alexei, Song, and Rafael, for your valuable feedback.

So, I understand that the key requirement here is to demonstrate a real-world scenario example
that can be effectively used in production environments
and to provide benchmark results.

Next up, I'll focus on developing a real-world use case for mobile devices
and providing test results.
Thanks again for the helpful insights.