| Message ID | 1436474552-31789-14-git-send-email-julien.grall@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 9 Jul 2015, Julien Grall wrote: > Only use the first 4KB of the page to store the events channel info. It > means that we will wast 60KB every time we allocate page for: ^ waste > * control block: a page is allocating per CPU > * event array: a page is allocating everytime we need to expand it > > I think we can reduce the memory waste for the 2 areas by: > > * control block: sharing between multiple vCPUs. Although it will > require some bookkeeping in order to not free the page when the CPU > goes offline and the other CPUs sharing the page still there > > * event array: always extend the array event by 64K (i.e 16 4K > chunk). That would require more care when we fail to expand the > event channel. But this is not implemented in this series, right? > Signed-off-by: Julien Grall <julien.grall@citrix.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: David Vrabel <david.vrabel@citrix.com> > --- > drivers/xen/events/events_base.c | 2 +- > drivers/xen/events/events_fifo.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c > index 96093ae..858d2f6 100644 > --- a/drivers/xen/events/events_base.c > +++ b/drivers/xen/events/events_base.c > @@ -40,11 +40,11 @@ > #include <asm/idle.h> > #include <asm/io_apic.h> > #include <asm/xen/pci.h> > -#include <xen/page.h> > #endif > #include <asm/sync_bitops.h> > #include <asm/xen/hypercall.h> > #include <asm/xen/hypervisor.h> > +#include <xen/page.h> > > #include <xen/xen.h> > #include <xen/hvm.h> Spurious change? > diff --git a/drivers/xen/events/events_fifo.c b/drivers/xen/events/events_fifo.c > index ed673e1..d53c297 100644 > --- a/drivers/xen/events/events_fifo.c > +++ b/drivers/xen/events/events_fifo.c > @@ -54,7 +54,7 @@ > > #include "events_internal.h" > > -#define EVENT_WORDS_PER_PAGE (PAGE_SIZE / sizeof(event_word_t)) > +#define EVENT_WORDS_PER_PAGE (XEN_PAGE_SIZE / sizeof(event_word_t)) > #define MAX_EVENT_ARRAY_PAGES (EVTCHN_FIFO_NR_CHANNELS / EVENT_WORDS_PER_PAGE) > > struct evtchn_fifo_queue { > -- > 2.1.4 >
Hi Stefano, On 16/07/2015 16:43, Stefano Stabellini wrote: > On Thu, 9 Jul 2015, Julien Grall wrote: >> Only use the first 4KB of the page to store the events channel info. It >> means that we will wast 60KB every time we allocate page for: > ^ waste > >> * control block: a page is allocating per CPU >> * event array: a page is allocating everytime we need to expand it >> >> I think we can reduce the memory waste for the 2 areas by: >> >> * control block: sharing between multiple vCPUs. Although it will >> require some bookkeeping in order to not free the page when the CPU >> goes offline and the other CPUs sharing the page still there >> >> * event array: always extend the array event by 64K (i.e 16 4K >> chunk). That would require more care when we fail to expand the >> event channel. > > But this is not implemented in this series, right? Yes, it's some ideas to improve the code. > > >> Signed-off-by: Julien Grall <julien.grall@citrix.com> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> >> Cc: David Vrabel <david.vrabel@citrix.com> >> --- >> drivers/xen/events/events_base.c | 2 +- >> drivers/xen/events/events_fifo.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c >> index 96093ae..858d2f6 100644 >> --- a/drivers/xen/events/events_base.c >> +++ b/drivers/xen/events/events_base.c >> @@ -40,11 +40,11 @@ >> #include <asm/idle.h> >> #include <asm/io_apic.h> >> #include <asm/xen/pci.h> >> -#include <xen/page.h> >> #endif >> #include <asm/sync_bitops.h> >> #include <asm/xen/hypercall.h> >> #include <asm/xen/hypervisor.h> >> +#include <xen/page.h> >> >> #include <xen/xen.h> >> #include <xen/hvm.h> > > Spurious change? No, xen/page.h was only included for x86 before. Now, it's included for every architecture. This is required in order to get XEN_PAGE_SIZE. Regards,
On Thu, 16 Jul 2015, Julien Grall wrote: > Hi Stefano, > > On 16/07/2015 16:43, Stefano Stabellini wrote: > > On Thu, 9 Jul 2015, Julien Grall wrote: > > > Only use the first 4KB of the page to store the events channel info. It > > > means that we will wast 60KB every time we allocate page for: > > ^ waste > > > > > * control block: a page is allocating per CPU > > > * event array: a page is allocating everytime we need to expand it > > > > > > I think we can reduce the memory waste for the 2 areas by: > > > > > > * control block: sharing between multiple vCPUs. Although it will > > > require some bookkeeping in order to not free the page when the CPU > > > goes offline and the other CPUs sharing the page still there > > > > > > * event array: always extend the array event by 64K (i.e 16 4K > > > chunk). That would require more care when we fail to expand the > > > event channel. > > > > But this is not implemented in this series, right? > > Yes, it's some ideas to improve the code. > > > > > > > > Signed-off-by: Julien Grall <julien.grall@citrix.com> > > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > Cc: David Vrabel <david.vrabel@citrix.com> > > > --- > > > drivers/xen/events/events_base.c | 2 +- > > > drivers/xen/events/events_fifo.c | 2 +- > > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/xen/events/events_base.c > > > b/drivers/xen/events/events_base.c > > > index 96093ae..858d2f6 100644 > > > --- a/drivers/xen/events/events_base.c > > > +++ b/drivers/xen/events/events_base.c > > > @@ -40,11 +40,11 @@ > > > #include <asm/idle.h> > > > #include <asm/io_apic.h> > > > #include <asm/xen/pci.h> > > > -#include <xen/page.h> > > > #endif > > > #include <asm/sync_bitops.h> > > > #include <asm/xen/hypercall.h> > > > #include <asm/xen/hypervisor.h> > > > +#include <xen/page.h> > > > > > > #include <xen/xen.h> > > > #include <xen/hvm.h> > > > > Spurious change? > > No, xen/page.h was only included for x86 before. Now, it's included for every > architecture. > > This is required in order to get XEN_PAGE_SIZE. Ah, right. Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
On 09/07/15 21:42, Julien Grall wrote: > Only use the first 4KB of the page to store the events channel info. It > means that we will wast 60KB every time we allocate page for: > * control block: a page is allocating per CPU > * event array: a page is allocating everytime we need to expand it Reviewed-by: David Vrabel <david.vrabel@citrix.com> > > I think we can reduce the memory waste for the 2 areas by: > > * control block: sharing between multiple vCPUs. Although it will > require some bookkeeping in order to not free the page when the CPU > goes offline and the other CPUs sharing the page still there > > * event array: always extend the array event by 64K (i.e 16 4K > chunk). That would require more care when we fail to expand the > event channel. I would extend it by 4 KiB each time but only allocate a new page every 16 times. This minimizes the resources used in Xen. David
Hi David, On 24/07/15 11:36, David Vrabel wrote: > On 09/07/15 21:42, Julien Grall wrote: >> Only use the first 4KB of the page to store the events channel info. It >> means that we will wast 60KB every time we allocate page for: >> * control block: a page is allocating per CPU >> * event array: a page is allocating everytime we need to expand it > > Reviewed-by: David Vrabel <david.vrabel@citrix.com> Thank you! >> >> I think we can reduce the memory waste for the 2 areas by: >> >> * control block: sharing between multiple vCPUs. Although it will >> require some bookkeeping in order to not free the page when the CPU >> goes offline and the other CPUs sharing the page still there >> >> * event array: always extend the array event by 64K (i.e 16 4K >> chunk). That would require more care when we fail to expand the >> event channel. > > I would extend it by 4 KiB each time but only allocate a new page every > 16 times. This minimizes the resources used in Xen. I will keep it in mind when I will send a patch to reduce memory waster. Regards,
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c index 96093ae..858d2f6 100644 --- a/drivers/xen/events/events_base.c +++ b/drivers/xen/events/events_base.c @@ -40,11 +40,11 @@ #include <asm/idle.h> #include <asm/io_apic.h> #include <asm/xen/pci.h> -#include <xen/page.h> #endif #include <asm/sync_bitops.h> #include <asm/xen/hypercall.h> #include <asm/xen/hypervisor.h> +#include <xen/page.h> #include <xen/xen.h> #include <xen/hvm.h> diff --git a/drivers/xen/events/events_fifo.c b/drivers/xen/events/events_fifo.c index ed673e1..d53c297 100644 --- a/drivers/xen/events/events_fifo.c +++ b/drivers/xen/events/events_fifo.c @@ -54,7 +54,7 @@ #include "events_internal.h" -#define EVENT_WORDS_PER_PAGE (PAGE_SIZE / sizeof(event_word_t)) +#define EVENT_WORDS_PER_PAGE (XEN_PAGE_SIZE / sizeof(event_word_t)) #define MAX_EVENT_ARRAY_PAGES (EVTCHN_FIFO_NR_CHANNELS / EVENT_WORDS_PER_PAGE) struct evtchn_fifo_queue {
Only use the first 4KB of the page to store the events channel info. It means that we will wast 60KB every time we allocate page for: * control block: a page is allocating per CPU * event array: a page is allocating everytime we need to expand it I think we can reduce the memory waste for the 2 areas by: * control block: sharing between multiple vCPUs. Although it will require some bookkeeping in order to not free the page when the CPU goes offline and the other CPUs sharing the page still there * event array: always extend the array event by 64K (i.e 16 4K chunk). That would require more care when we fail to expand the event channel. Signed-off-by: Julien Grall <julien.grall@citrix.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: David Vrabel <david.vrabel@citrix.com> --- drivers/xen/events/events_base.c | 2 +- drivers/xen/events/events_fifo.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)