Re: [RFC PATCH v8 2/4] selftests/bpf: add iocost cost model test

[Tao Cui]

Hello, Tejun.

在 2026/10/1 08:20, Tejun Heo 写道:
> Hello, Tao.
> 
> The following is a Claude-generated review.
> 
> On Wed, 30 Sep 2026 15:51:52 +0800, Tao Cui wrote:
>> Add an example cost model implementing the full builtin linear HDD
>> formula at double cost, and a test which attaches it to one device:
>> the dev member of the struct_ops is written through the map's
>> initial value before load, as hid_bpf tests do with hid_id, and
>> loading the struct_ops attaches the model to the device.  The test
>> verifies the ctrl=bpf readback while attached, that a second model
>> on the same device fails with -EBUSY, and that detaching restores
>> the builtin model.
> ...
>> cgroup that comes back starts fresh.  opf carries the full bio->bi_opf
>> including REQ_* flag bits, so the operation must be extracted with a
>> mask, not compared for equality.
> 
> The description is out of date. The test reads back model=bpf and checks
> that ctrl=bpf is rejected, calc_cost() takes the bio so there is no opf
> argument, attaching rather than loading binds the model, and the
> multi-stream model and its test aren't mentioned at all.
> 

The commit message is rewritten to describe the current test,
including the multi-stream model.

>> +	if (fwrite(buf, 1, strlen(buf), fp) != strlen(buf))
>> +		err = ferror(fp) ? errno : EIO;
>> +	if (fclose(fp) && !err)
>> +		err = errno;
>> +	return err;
> ...
>> +		err = write_cost_model(dev, "ctrl=bpf");
>> +		ASSERT_ERR(err, "ctrl_bpf_rejected");
> 
> write_cost_model() returns a positive errno while ASSERT_ERR() wants a
> negative value, so this and model_bpf_after_detach fail with "unexpected
> success: 22" on a correct kernel. Return -errno. Can you double check
> that the posted runner passes against the posted kernel?
> 

Fixed, it returns -errno. The posted runner had not been run against
the posted kernel; it now passes against the v9 kernel.

>> +	/* dev is the first member of struct iocost_model_ops */
>> +	ops_dev = bpf_map__initial_value(skel->maps.iocost_2x, NULL);
> 
> The skeleton exposes the struct_ops shadow type, so
> skel->struct_ops.iocost_2x->dev = ... is type checked and drops the
> layout assumption. Same for iocost_ms.
> 

Done for both models.

>> +SEC(".struct_ops")
>> +struct iocost_model_ops iocost_2x = {
> 
> With a plain struct_ops map, a test that dies between attach and detach
> leaves the model attached until something deletes the map element. The
> hid and sched_ext selftests use ".struct_ops.link" so that closing the fd
> detaches. Can you use that here too?
> 

Done, both maps are ".struct_ops.link".

>> +	cur = *cursor;
>> +	if (cur && priced) {
>> +		seek_pages = sector > cur ? sector - cur
>> +					   : cur - sector;
> 
> A dataless flush is REQ_OP_WRITE|REQ_PREFLUSH at sector 0 with bi_size 0,
> so priced is set here. Once a cgroup's cursor is past 16MB, every fsync
> is judged a random write and charged 2 * (WRANDIO + WPAGE), about 5ms,
> not the one-page write the header comment describes, and iocost_ms.c
> prices the same bio with the sequential base. Can you gate the seek
> judgement on a non-zero size?
> 

Done: the seek judgement is now gated on a non-zero IO size, so a
dataless flush is priced as a size-zero sequential write, matching the
builtin model.

Thanks.
Tao

> Thanks.
> 
> --
> tejun