tools/libxengnttab: correct size of allocated memory

[PATCH] tools/libxengnttab: correct size of allocated memory

Posted by Juergen Gross 3 years, 11 months ago

The size of the memory allocated for the IOCTL_GNTDEV_MAP_GRANT_REF
ioctl() parameters is calculated wrong, which results in too much
memory allocated.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libs/gnttab/freebsd.c | 2 +-
 tools/libs/gnttab/linux.c   | 8 +++-----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
index 886b588303..0588501d0f 100644
--- a/tools/libs/gnttab/freebsd.c
+++ b/tools/libs/gnttab/freebsd.c
@@ -74,7 +74,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
     void *addr = NULL;
     int domids_stride;
     unsigned int refs_size = ROUNDUP(count *
-                                     sizeof(struct ioctl_gntdev_map_grant_ref),
+                                     sizeof(struct ioctl_gntdev_grant_ref),
                                      PAGE_SHIFT);
 
     domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index a01bb6c698..74331a4c7b 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -91,9 +91,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 {
     int fd = xgt->fd;
     struct ioctl_gntdev_map_grant_ref *map;
-    unsigned int map_size = ROUNDUP((sizeof(*map) + (count - 1) *
-                                    sizeof(struct ioctl_gntdev_map_grant_ref)),
-                                    PAGE_SHIFT);
+    unsigned int map_size = sizeof(*map) + (count - 1) * sizeof(map->refs[0]);
     void *addr = NULL;
     int domids_stride = 1;
     int i;
@@ -102,10 +100,10 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
         domids_stride = 0;
 
     if ( map_size <= PAGE_SIZE )
-        map = alloca(sizeof(*map) +
-                     (count - 1) * sizeof(struct ioctl_gntdev_map_grant_ref));
+        map = alloca(map_size);
     else
     {
+        map_size = ROUNDUP(map_size, PAGE_SHIFT);
         map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
                    MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
         if ( map == MAP_FAILED )
-- 
2.26.1

Re: [PATCH] tools/libxengnttab: correct size of allocated memory

Posted by Ian Jackson 3 years, 11 months ago

Juergen Gross writes ("[PATCH] tools/libxengnttab: correct size of allocated memory"):
> The size of the memory allocated for the IOCTL_GNTDEV_MAP_GRANT_REF
> ioctl() parameters is calculated wrong, which results in too much
> memory allocated.

Added Roger to CC.

Firstly,

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thank you.


But, looking at this code, why on earth what the ?

The FreeBSD code checks to see if it's less than a page and if so uses
malloc and otherwise uses mmap !  Why not unconditionally use malloc ?

Likewise, the Linux code has its own mmap-based memory-obtainer.  ISTM
that malloc is probably going to be better.  Often it will be able to
give out even a substantial amount without making a syscall.

Essentially, we have two (similar but not identical) tiny custom
memory allocators here.  Also, the Linux and FreeBSD code are
remarkably similar which bothers me.

Anyway, these observations are no criticism of Juergen's patch.

Regards,
Ian.

Re: [PATCH] tools/libxengnttab: correct size of allocated memory

Posted by Roger Pau Monné 3 years, 11 months ago

On Wed, May 20, 2020 at 03:49:59PM +0100, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH] tools/libxengnttab: correct size of allocated memory"):
> > The size of the memory allocated for the IOCTL_GNTDEV_MAP_GRANT_REF
> > ioctl() parameters is calculated wrong, which results in too much
> > memory allocated.
> 
> Added Roger to CC.
> 
> Firstly,
> 
> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

For the FreeBSD bits:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> 
> Thank you.
> 
> 
> But, looking at this code, why on earth what the ?
> 
> The FreeBSD code checks to see if it's less than a page and if so uses
> malloc and otherwise uses mmap !  Why not unconditionally use malloc ?
> 
> Likewise, the Linux code has its own mmap-based memory-obtainer.  ISTM
> that malloc is probably going to be better.  Often it will be able to
> give out even a substantial amount without making a syscall.
> 
> Essentially, we have two (similar but not identical) tiny custom
> memory allocators here.  Also, the Linux and FreeBSD code are
> remarkably similar which bothers me.

Right. This is due to the FreeBSD file being mostly a clone of the
Linux one. I agree the duplication could be abstracted away.

I really have no idea why malloc or mmap is used, maybe at some point
requesting regions > PAGE_SIZE was considered faster using mmap
directly?

Thanks, Roger.

Re: [PATCH] tools/libxengnttab: correct size of allocated memory

Posted by Jürgen Groß 3 years, 10 months ago

On 20.05.20 17:56, Roger Pau Monné wrote:
> On Wed, May 20, 2020 at 03:49:59PM +0100, Ian Jackson wrote:
>> Juergen Gross writes ("[PATCH] tools/libxengnttab: correct size of allocated memory"):
>>> The size of the memory allocated for the IOCTL_GNTDEV_MAP_GRANT_REF
>>> ioctl() parameters is calculated wrong, which results in too much
>>> memory allocated.
>>
>> Added Roger to CC.
>>
>> Firstly,
>>
>> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> For the FreeBSD bits:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Any reason the patch didn't go in yet?


Juergen

RE: [PATCH] tools/libxengnttab: correct size of allocated memory

Posted by Paul Durrant 3 years, 10 months ago

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jürgen Groß
> Sent: 11 June 2020 16:26
> To: Roger Pau Monné <roger.pau@citrix.com>; Ian Jackson <ian.jackson@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH] tools/libxengnttab: correct size of allocated memory
> 
> On 20.05.20 17:56, Roger Pau Monné wrote:
> > On Wed, May 20, 2020 at 03:49:59PM +0100, Ian Jackson wrote:
> >> Juergen Gross writes ("[PATCH] tools/libxengnttab: correct size of allocated memory"):
> >>> The size of the memory allocated for the IOCTL_GNTDEV_MAP_GRANT_REF
> >>> ioctl() parameters is calculated wrong, which results in too much
> >>> memory allocated.
> >>
> >> Added Roger to CC.
> >>
> >> Firstly,
> >>
> >> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> >
> > For the FreeBSD bits:
> >
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Any reason the patch didn't go in yet?
> 

I'd be quite happy for this to go in now, consider it

Release-acked-by: Paul Durrant <paul@xen.org>

> 
> Juergen

RE: [PATCH] tools/libxengnttab: correct size of allocated memory

Posted by Ian Jackson 3 years, 10 months ago

Paul Durrant writes ("RE: [PATCH] tools/libxengnttab: correct size of allocated memory"):
> > -----Original Message-----
> > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jürgen Groß
> > Sent: 11 June 2020 16:26
> > To: Roger Pau Monné <roger.pau@citrix.com>; Ian Jackson <ian.jackson@citrix.com>
> > Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>
> > Subject: Re: [PATCH] tools/libxengnttab: correct size of allocated memory
> > 
> > On 20.05.20 17:56, Roger Pau Monné wrote:
> > > On Wed, May 20, 2020 at 03:49:59PM +0100, Ian Jackson wrote:
> > >> Juergen Gross writes ("[PATCH] tools/libxengnttab: correct size of allocated memory"):
> > >>> The size of the memory allocated for the IOCTL_GNTDEV_MAP_GRANT_REF
> > >>> ioctl() parameters is calculated wrong, which results in too much
> > >>> memory allocated.
> > >>
> > >> Added Roger to CC.
> > >>
> > >> Firstly,
> > >>
> > >> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > >
> > > For the FreeBSD bits:
> > >
> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > Any reason the patch didn't go in yet?
> > 
> 
> I'd be quite happy for this to go in now, consider it
> 
> Release-acked-by: Paul Durrant <paul@xen.org>

Thanks.  I tried to apply this but it doesn't seem to apply to
staging.  Jürgen, would you care to rebase/resend ?

Thanks,
Ian.

Re: [PATCH] tools/libxengnttab: correct size of allocated memory

Posted by Andrew Cooper 3 years, 10 months ago

On 15/06/2020 15:07, Ian Jackson wrote:
> Paul Durrant writes ("RE: [PATCH] tools/libxengnttab: correct size of allocated memory"):
>>> -----Original Message-----
>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jürgen Groß
>>> Sent: 11 June 2020 16:26
>>> To: Roger Pau Monné <roger.pau@citrix.com>; Ian Jackson <ian.jackson@citrix.com>
>>> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>
>>> Subject: Re: [PATCH] tools/libxengnttab: correct size of allocated memory
>>>
>>> On 20.05.20 17:56, Roger Pau Monné wrote:
>>>> On Wed, May 20, 2020 at 03:49:59PM +0100, Ian Jackson wrote:
>>>>> Juergen Gross writes ("[PATCH] tools/libxengnttab: correct size of allocated memory"):
>>>>>> The size of the memory allocated for the IOCTL_GNTDEV_MAP_GRANT_REF
>>>>>> ioctl() parameters is calculated wrong, which results in too much
>>>>>> memory allocated.
>>>>> Added Roger to CC.
>>>>>
>>>>> Firstly,
>>>>>
>>>>> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
>>>> For the FreeBSD bits:
>>>>
>>>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>>> Any reason the patch didn't go in yet?
>>>
>> I'd be quite happy for this to go in now, consider it
>>
>> Release-acked-by: Paul Durrant <paul@xen.org>
> Thanks.  I tried to apply this but it doesn't seem to apply to
> staging.  Jürgen, would you care to rebase/resend ?

I already committed it, seeing as it was fully acked.

https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=aad20e538d7ba0e36f5ed8d2bebb74096e3a6d7f

~Andrew

Re: [PATCH] tools/libxengnttab: correct size of allocated memory

Posted by Ian Jackson 3 years, 10 months ago

Andrew Cooper writes ("Re: [PATCH] tools/libxengnttab: correct size of allocated memory"):
> I already committed it, seeing as it was fully acked.
> 
> https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=aad20e538d7ba0e36f5ed8d2bebb74096e3a6d7f

That must be why it didn't apply :-).

Thanks,
Ian.