Re: [RFC 05/27] vhost: add helper for IOTLB entries shared page check

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Mike,

On 4/17/23 21:39, Mike Pattrick wrote:
> Hi Maxime,
> 
> On Fri, Mar 31, 2023 at 11:43 AM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> This patch introduces a helper to check whether two IOTLB
>> entries share a page.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   lib/vhost/iotlb.c | 25 ++++++++++++++++++++-----
>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/vhost/iotlb.c b/lib/vhost/iotlb.c
>> index e8f1cb661e..d919f74704 100644
>> --- a/lib/vhost/iotlb.c
>> +++ b/lib/vhost/iotlb.c
>> @@ -23,6 +23,23 @@ struct vhost_iotlb_entry {
>>
>>   #define IOTLB_CACHE_SIZE 2048
>>
>> +static bool
>> +vhost_user_iotlb_share_page(struct vhost_iotlb_entry *a, struct vhost_iotlb_entry *b,
>> +               uint64_t align)
>> +{
>> +       uint64_t a_end, b_start;
>> +
>> +       if (a == NULL || b == NULL)
>> +               return false;
>> +
>> +       /* Assumes entry a lower than entry b */
>> +       RTE_ASSERT(a->uaddr < b->uaddr);
>> +       a_end = RTE_ALIGN_CEIL(a->uaddr + a->size, align);
>> +       b_start = RTE_ALIGN_FLOOR(b->uaddr, align);
>> +
>> +       return a_end > b_start;
> 
> This may be a very minor detail, but it might improve readability if
> the a_end calculation used addr + size - 1 and the return conditional
> used >=.

That's actually how I initially implemented it, but discussing with
David we agreed it would be better that way for consistency with
vhost_user_iotlb_clear_dump().

Regards,
Maxime

> 
> Cheers,
> M
> 
> 
>> +}
>> +
>>   static void
>>   vhost_user_iotlb_set_dump(struct virtio_net *dev, struct vhost_iotlb_entry *node)
>>   {
>> @@ -37,16 +54,14 @@ static void
>>   vhost_user_iotlb_clear_dump(struct virtio_net *dev, struct vhost_iotlb_entry *node,
>>                  struct vhost_iotlb_entry *prev, struct vhost_iotlb_entry *next)
>>   {
>> -       uint64_t align, mask;
>> +       uint64_t align;
>>
>>          align = hua_to_alignment(dev->mem, (void *)(uintptr_t)node->uaddr);
>> -       mask = ~(align - 1);
>>
>>          /* Don't disable coredump if the previous node is in the same page */
>> -       if (prev == NULL || (node->uaddr & mask) != ((prev->uaddr + prev->size - 1) & mask)) {
>> +       if (!vhost_user_iotlb_share_page(prev, node, align)) {
>>                  /* Don't disable coredump if the next node is in the same page */
>> -               if (next == NULL ||
>> -                               ((node->uaddr + node->size - 1) & mask) != (next->uaddr & mask))
>> +               if (!vhost_user_iotlb_share_page(node, next, align))
>>                          mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, false, align);
>>          }
>>   }
>> --
>> 2.39.2
>>
>