| Message ID | 20200520083501.31704-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/libxengnttab: correct size of allocated memory | expand |
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.
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.
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
> -----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
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.
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
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.
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 )
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(-)