diff mbox series

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

Message ID 20230331154259.1447831-6-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series Add VDUSE support to Vhost library |

Commit Message

Maxime Coquelin March 31, 2023, 3:42 p.m. UTC 
  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(-)

Comments

Mike Pattrick April 17, 2023, 7:39 p.m. UTC | #1 
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 >=.


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
>
Maxime Coquelin April 19, 2023, 9:35 a.m. UTC | #2 
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
>>
>
Mike Pattrick April 19, 2023, 2:52 p.m. UTC | #3 
On Wed, Apr 19, 2023 at 5:35 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> 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().

I can get behind consistency.

Acked-by: Mike Pattrick <mkp@redhat.com>

>
> 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
> >>
> >
>
Chenbo Xia April 24, 2023, 2:59 a.m. UTC | #4 
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, March 31, 2023 11:43 PM
> To: dev@dpdk.org; david.marchand@redhat.com; Xia, Chenbo
> <chenbo.xia@intel.com>; mkp@redhat.com; fbl@redhat.com;
> jasowang@redhat.com; Liang, Cunming <cunming.liang@intel.com>; Xie, Yongji
> <xieyongji@bytedance.com>; echaudro@redhat.com; eperezma@redhat.com;
> amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [RFC 05/27] vhost: add helper for IOTLB entries shared page check
> 
> 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;
> +}
> +
>  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

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
diff mbox series

Patch

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;
+}
+
 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);
 	}
 }