On 01/09/2022 21:36, Alex Williamson wrote:
> On Thu, 1 Sep 2022 20:39:40 +0100
> joao.m.martins@xxxxxxxxxx wrote:
>
>> On 01/09/2022 19:47, Alex Williamson wrote:
>>> On Thu, 1 Sep 2022 12:38:47 +0300
>>> Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
>>>> + * An example of the APIs on how to use/iterate over the IOVA bitmap:
>>>> + *
>>>> + * bitmap = iova_bitmap_alloc(iova, length, page_size, data);
>>>> + * if (IS_ERR(bitmap))
>>>> + * return PTR_ERR(bitmap);
>>>> + *
>>>> + * ret = iova_bitmap_for_each(bitmap, arg, dirty_reporter_fn);
>>>> + *
>>>> + * iova_bitmap_free(bitmap);
>>>> + *
>>>> + * An implementation of the lower end (referred to above as
>>>> + * dirty_reporter_fn to exemplify), that is tracking dirty bits would mark
>>>> + * an IOVA as dirty as following:
>>>> + * iova_bitmap_set(bitmap, iova, page_size);
>>>> + * Or a contiguous range (example two pages):
>>>> + * iova_bitmap_set(bitmap, iova, 2 * page_size);
>>>
>>> This seems like it implies a stronger correlation to the
>>> iova_bitmap_alloc() page_size than actually exists. The implementation
>>> of the dirty_reporter_fn() may not know the reporting page_size. The
>>> value here is just a size_t and iova_bitmap handles the rest, right?
>>>
>> Correct.
>>
>> The intent was to show an example of what the different usage have
>> an effect in the end bitmap data (1 page and then 2 pages). An alternative
>> would be:
>>
>> An implementation of the lower end (referred to above as
>> dirty_reporter_fn to exemplify), that is tracking dirty bits would mark
>> an IOVA range spanning @iova_length as dirty, using the configured
>> @page_size:
>>
>> iova_bitmap_set(bitmap, iova, iova_length)
>>
>> But with a different length variable (i.e. iova_length) to avoid being confused with
>> the length in iova_bitmap_alloc right before this paragraph. But the example in the
>> patch looks a bit more clear on the outcomes to me personally.
>
> How about:
>
> Each iteration of the dirty_reporter_fn is called with a unique @iova
> and @length argument, indicating the current range available through
> the iova_bitmap. The dirty_reporter_fn uses iova_bitmap_set() to
> mark dirty areas within that provided range
>
> ?
>
Yeah, much clearer. Perhaps I'll add a : and the API usage like this:
Each iteration of the dirty_reporter_fn is called with a unique @iova
and @length argument, indicating the current range available through
the iova_bitmap. The dirty_reporter_fn uses iova_bitmap_set() to
mark dirty areas (@iova_length) within that provided range as following:
iova_bitmap_set(bitmap, iova, iova_length)
And of course I'll change this in the commit message.
> ...
>>>> +/**
>>>> + * iova_bitmap_for_each() - Iterates over the bitmap
>>>> + * @bitmap: IOVA bitmap to iterate
>>>> + * @opaque: Additional argument to pass to the callback
>>>> + * @fn: Function that gets called for each IOVA range
>>>> + *
>>>> + * Helper function to iterate over bitmap data representing a portion of IOVA
>>>> + * space. It hides the complexity of iterating bitmaps and translating the
>>>> + * mapped bitmap user pages into IOVA ranges to process.
>>>> + *
>>>> + * Return: 0 on success, and an error on failure either upon
>>>> + * iteration or when the callback returns an error.
>>>> + */
>>>> +int iova_bitmap_for_each(struct iova_bitmap *bitmap, void *opaque,
>>>> + int (*fn)(struct iova_bitmap *bitmap,
>>>> + unsigned long iova, size_t length,
>>>> + void *opaque))
>>>
>>> It might make sense to typedef an iova_bitmap_fn_t in the header to use
>>> here.
>>>
>> OK, will do. I wasn't sure which style was preferred so went with simplest on
>> first take.
>
> It looks like it would be a little cleaner, but yeah, probably largely
> style.
>
/me nods
>>>> +{
>>>> + int ret = 0;
>>>> +
>>>> + for (; !iova_bitmap_done(bitmap) && !ret;
>>>> + ret = iova_bitmap_advance(bitmap)) {
>>>> + ret = fn(bitmap, iova_bitmap_mapped_iova(bitmap),
>>>> + iova_bitmap_mapped_length(bitmap), opaque);
>>>> + if (ret)
>>>> + break;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +/**
>>>> + * iova_bitmap_set() - Records an IOVA range in bitmap
>>>> + * @bitmap: IOVA bitmap
>>>> + * @iova: IOVA to start
>>>> + * @length: IOVA range length
>>>> + *
>>>> + * Set the bits corresponding to the range [iova .. iova+length-1] in
>>>> + * the user bitmap.
>>>> + *
>>>> + * Return: The number of bits set.
>>>
>>> Is this relevant to the caller?
>>>
>> The thinking that number of bits was a way for caller to validate that
>> 'some bits' was set, i.e. sort of error return value. But none of the callers
>> use it today, it's true. Suppose I should remove it, following bitmap_set()
>> returning void too.
>
> I think 0/-errno are sufficient if we need an error path, otherwise
> void is fine. As above, the reporter fn isn't strongly tied to the
> page size of the bitmap, so number of bits just didn't make sense to me.
>
OK, I am dropping it for now.
