RFC: ABD chunk iterator #16848
RFC: ABD chunk iterator #16848
Conversation
The existing iterators do not have any way for an external user to get the size of the data within the current iteration. This lifts that out of abd_iter_map() and exposes it as abd_iter_size() for the chunk iterator to use. FreeBSD not included for now, since this is just for my own testing.
For now, these are implemented in terms of the old iterators.
There is no real overhead change here, because it still has to call the callback function, which has to manage its own state, but the code is much simpler.
This shows how this form of iterator can be much simpler to work with. No need to set up separate state struct and callback function, just write a loop.
This version is not really simpler. (Though it did help me to see a small efficiency gain: the existing version will remap each chunk when either iterator advances. This version does not). I suspect that users of abd_iterate_func2 will be able to do a better job by using chunk iterators directly, rather than through callbacks, because they know more about the incoming data format and how and when to advance it.
This is where I really wanted to get to. Rather then needing the loop boilerplate every time, have a macro that hide all that away, having a use that looks more like a plain old for loop. This method has two downsides: - the data and size vars have to be declared outside the loop - an early exit (break or return) is not possible, as there's no place to unmap the chunk.
By making the loop block an arg to the macro, we can wrap it, and solve both the problems in the previous version. We can declare the data & size vars ourselves, so the caller doesn't have to, and we can run at loop exit, so can unmap the chunk properly (alas, we can't do it for an early return, but that's a much rarer thing to want than an early break). The cost is a tiny bit of readbility, as a code block as a macro arg is a little less familiar than one following a loop operator. The extra safety seems worth it to me.
amotin
added
the
Status: Design Review Needed
| * - void abd_chunk_advance(abd_chunk_t *ch) | ||
| * | ||
| * Move the iterator to the next chunk. If there is no next chunk, the | ||
| * iterator is changed to the "done" state and is no longer useable. | ||
| * | ||
| * - boolean_t abd_chunk_done(abd_chunk_t *ch) | ||
| * | ||
| * If true, the iterator is pointing to a valid chunk, and the underlying | ||
| * memory can be accessed with the access functions. If false, the iterator | ||
| * is exhausted and no longer useable. |
There was a problem hiding this comment.
Definitions of "done" in those paragraphs seems to be opposite.
The state structure seems not going anywhere. Ability to avoid callbacks and do thing inline indeed makes it much more flexible, but the macro style makes me shiver similar to style checker. I like the direction, but not so much the specific implementation.
I am not sure you are doing much about it here. For now you just included the old iterator with its code as is, but I worry that once you try to integrate it we'll end up about where we are now. We do need some OS-specific ABD-specific storage for mapping, etc.
It seems current code already operates in chunks of possible mappings. The fact that |
[Sponsors: Klara, Inc., Wasabi Technology, Inc.]
Motivation and Context
This is a prototype of "chunk" iterators for ABDs.
This is attempting to solve the problems (which of course I assert are; we can discuss that if you wish):
This PR proposes an alternative iterator that expressly yields "chunks", that is, an object (struct) representing an arbitrary run of bytes. The caller can than take operations against the chunk (request size, map, unmap, etc) separately. All iterator movement then becomes simply moving to the next chunk. The much simpler housekeeping required allows an iterator to be used directly rather than through a control function, addressing the usability concerns.
For this PR I'm looking for rough consensus on the goals and interface.
Description
For easy reading online:
abd_cmp_buf_off)See the individual commits and their comments for more details. In the commits and their comments:
abd_iterate_funcin terms of chunk iteratorsabd_iterate_functo use a simpler iterator loopabd_iterate_func2in terms of chunk iteratorsabd_for_each_chunkmacro that hides the details of the most common style of iterator loop (map each chunk)This PR is largely aimed at getting the interface right; the implementation details are unimportant for now. My intention when/if this passes muster is to entirely remove the existing iterator code, including the callback iterators, so none of this implementation will carry over. So don't worry about it, but do point out anything you think might not be implementable.
The only thing not here is a replacement for
abd_iterate_page_func(). I don't think it's hard, just a bit fiddly - needs a way to get the page pointer and data offset/size within for a chunk, and a way to indicate that each chunk should be a page, even if it could be bigger (linear), and what to do with compound pages. Maybeabd_chunk_start()will gain aflagsparameter. I'd see what falls out of the "real" implementation and what feels right. I don't want to overthink it, but I still want it to be usable - more easily sharing pages with the kernel is gonna be useful!How Has This Been Tested?
Light sanity runs, as befits a prototype.
Types of changes
One or more of:
Checklist:
Signed-off-by.