Date:   Fri, 2 Sep 2022 10:42:41 +0100
From:   Joao Martins <joao.m.martins@...cle.com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     Yishai Hadas <yishaih@...dia.com>, jgg@...dia.com,
        saeedm@...dia.com, kvm@...r.kernel.org, netdev@...r.kernel.org,
        kuba@...nel.org, kevin.tian@...el.com, leonro@...dia.com,
        maorg@...dia.com, cohuck@...hat.com
Subject: Re: [PATCH V5 vfio 04/10] vfio: Add an IOVA bitmap support

On 01/09/2022 21:36, Alex Williamson wrote:
> On Thu, 1 Sep 2022 20:39:40 +0100
> joao.m.martins@...cle.com wrote:
> 
>> On 01/09/2022 19:47, Alex Williamson wrote:
>>> On Thu, 1 Sep 2022 12:38:47 +0300
>>> Yishai Hadas <yishaih@...dia.com> 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.

Powered by blists - more mailing lists