[Patch] by default don't give all memory to dom0

[Patch] by default don't give all memory to dom0

[Siddha, Suresh B]

On a system where swiotlb is used, by default xen fails to allocate
memory for dom0's swiotlb contiguous memory request.

By default, xen needs to reserve some portion of memory to satisfy
SWIOTLB and other contguous memory region requests. Following
the current swiotlb enabling mechanism, Appended patch reserves 128MB 
of memory on systems with more than 2GB of RAM.

Ideally shouldn't we enable SWIOTLB in dom0 and this DMA memory reservation
in hypervisor by default? Otherwise we will have a problem(even on systems 
with less than 2GB of RAM) in servicing a driver DMA request to a 
kmalloc'd buffer which spans more than a page or the various 
xen_create_contiguous_region() requests.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>

diff -r 84ee014ebd41 xen/arch/x86/domain_build.c
--- a/xen/arch/x86/domain_build.c	Wed Aug 17 20:34:38 2005
+++ b/xen/arch/x86/domain_build.c	Thu Aug 18 11:52:57 2005
@@ -34,6 +34,18 @@
         opt_dom0_mem = (unsigned int)(bytes >> 10);
 }
 custom_param("dom0_mem", parse_dom0_mem);
+
+static unsigned int reserve_dmapages = 0;
+static void parse_reserve_dmamem(char *s)
+{
+    unsigned long long bytes = parse_size_and_unit(s);
+    /* If no unit is specified we default to kB units, not bytes. */
+    if ( isdigit(s[strlen(s)-1]) )
+        reserve_dmapages = (unsigned int)bytes >> (PAGE_SHIFT - 10);
+    else
+        reserve_dmapages = (unsigned int)(bytes >> PAGE_SHIFT);
+}
+custom_param("dma_mem", parse_reserve_dmamem);
 
 static unsigned int opt_dom0_shadow = 0;
 boolean_param("dom0_shadow", opt_dom0_shadow);
@@ -137,10 +149,13 @@
 
     printk("*** LOADING DOMAIN 0 ***\n");
 
-    /* By default DOM0 is allocated all available memory. */
     d->max_pages = ~0U;
+    /* By default DOM0 is allocated (all available memory - reserved dma 
+       low memory) */
+    if (!reserve_dmapages && max_page > 0x7ffff)
+	reserve_dmapages = 32 * 1024; /* 128MB */
     if ( (nr_pages = opt_dom0_mem >> (PAGE_SHIFT - 10)) == 0 )
-        nr_pages = avail_domheap_pages() +
+        nr_pages = avail_domheap_pages() - reserve_dmapages +
             ((initrd_len + PAGE_SIZE - 1) >> PAGE_SHIFT) +
             ((image_len  + PAGE_SIZE - 1) >> PAGE_SHIFT);

Re: [Patch] by default don't give all memory to dom0

[David Hopwood]

Siddha, Suresh B wrote:
[...]

> +static void parse_reserve_dmamem(char *s)
> +{
> +    unsigned long long bytes = parse_size_and_unit(s);
> +    /* If no unit is specified we default to kB units, not bytes. */
> +    if ( isdigit(s[strlen(s)-1]) )
> +        reserve_dmapages = (unsigned int)bytes >> (PAGE_SHIFT - 10);
> +    else
> +        reserve_dmapages = (unsigned int)(bytes >> PAGE_SHIFT);
> +}

Shouldn't the default unit be consistent for all Xen memory parameters?

-- 
David Hopwood <david.nospam.hopwood@blueyonder.co.uk>

Re: [Patch] by default don't give all memory to dom0

[Siddha, Suresh B]

On Thu, Aug 18, 2005 at 08:50:57PM +0100, David Hopwood wrote:
> Siddha, Suresh B wrote:
> [...]
> 
> > +static void parse_reserve_dmamem(char *s)
> > +{
> > +    unsigned long long bytes = parse_size_and_unit(s);
> > +    /* If no unit is specified we default to kB units, not bytes. */
> > +    if ( isdigit(s[strlen(s)-1]) )
> > +        reserve_dmapages = (unsigned int)bytes >> (PAGE_SHIFT - 10);
> > +    else
> > +        reserve_dmapages = (unsigned int)(bytes >> PAGE_SHIFT);
> > +}
> 
> Shouldn't the default unit be consistent for all Xen memory parameters?

I am just following the dom0_mem conventions for which default unit is kB.
Am I missing something?

thanks,
suresh

Re: [Patch] by default don't give all memory to dom0

[David Hopwood]

Siddha, Suresh B wrote:
> On Thu, Aug 18, 2005 at 08:50:57PM +0100, David Hopwood wrote:
>>Siddha, Suresh B wrote:
>>[...]
>>
>>>+static void parse_reserve_dmamem(char *s)
>>>+{
>>>+    unsigned long long bytes = parse_size_and_unit(s);
>>>+    /* If no unit is specified we default to kB units, not bytes. */
>>>+    if ( isdigit(s[strlen(s)-1]) )
>>>+        reserve_dmapages = (unsigned int)bytes >> (PAGE_SHIFT - 10);
>>>+    else
>>>+        reserve_dmapages = (unsigned int)(bytes >> PAGE_SHIFT);
>>>+}
>>
>>Shouldn't the default unit be consistent for all Xen memory parameters?
> 
> I am just following the dom0_mem conventions for which default unit is kB.
> Am I missing something?

If Kbytes is to be the default unit, then that default should probably be
encoded in 'parse_size_and_unit', rather than having code like the above
convert from bytes to Kbytes in many places.

-- 
David Hopwood <david.nospam.hopwood@blueyonder.co.uk>

Re: [Patch] by default don't give all memory to dom0

[Siddha, Suresh B]

On Thu, Aug 18, 2005 at 10:55:53PM +0100, David Hopwood wrote:
> Siddha, Suresh B wrote:
> > On Thu, Aug 18, 2005 at 08:50:57PM +0100, David Hopwood wrote:
> >>Siddha, Suresh B wrote:
> >>[...]
> >>
> >>>+static void parse_reserve_dmamem(char *s)
> >>>+{
> >>>+    unsigned long long bytes = parse_size_and_unit(s);
> >>>+    /* If no unit is specified we default to kB units, not bytes. */
> >>>+    if ( isdigit(s[strlen(s)-1]) )
> >>>+        reserve_dmapages = (unsigned int)bytes >> (PAGE_SHIFT - 10);
> >>>+    else
> >>>+        reserve_dmapages = (unsigned int)(bytes >> PAGE_SHIFT);
> >>>+}
> >>
> >>Shouldn't the default unit be consistent for all Xen memory parameters?
> > 
> > I am just following the dom0_mem conventions for which default unit is kB.
> > Am I missing something?
> 
> If Kbytes is to be the default unit, then that default should probably be
> encoded in 'parse_size_and_unit', rather than having code like the above
> convert from bytes to Kbytes in many places.

For "mem=" option, default unit is bytes. I think we should go with the
linux style and make the default unit as bytes. If no one has any objections,
then I can send a patch for it.

thanks,
suresh

Re: [Patch] by default don't give all memory to dom0

[Keir Fraser]

On 19 Aug 2005, at 01:13, Siddha, Suresh B wrote:

>> If Kbytes is to be the default unit, then that default should 
>> probably be
>> encoded in 'parse_size_and_unit', rather than having code like the 
>> above
>> convert from bytes to Kbytes in many places.
>
> For "mem=" option, default unit is bytes. I think we should go with the
> linux style and make the default unit as bytes. If no one has any 
> objections,
> then I can send a patch for it.

Changing it will annoy people who are used to 2.0-style dom0_mem= 
parameter. There is no good reason to change it: it's documented and, 
if you want to avoid confusion, just stick a unit symbol on the end.

  -- Keir

Re: [Patch] by default don't give all memory to dom0

[Keir Fraser]

On 18 Aug 2005, at 20:24, Siddha, Suresh B wrote:

> By default, xen needs to reserve some portion of memory to satisfy
> SWIOTLB and other contguous memory region requests. Following
> the current swiotlb enabling mechanism, Appended patch reserves 128MB
> of memory on systems with more than 2GB of RAM.

Hmmm... sounds reasonable. I'd rather have one dom0 memory parameter 
though --- keeping dom0_mem but have +ve value mean 'allocate this 
amount' and -ve mean 'allocate full memory - this amount'. Otherwise we 
have two competing parameters specifying basically the same thing...

I don't much like hacky 'policies' that hardcode default reservations 
in the hypervisor, but I think this one is pretty sensible.

> Ideally shouldn't we enable SWIOTLB in dom0 and this DMA memory 
> reservation
> in hypervisor by default? Otherwise we will have a problem(even on 
> systems
> with less than 2GB of RAM) in servicing a driver DMA request to a
> kmalloc'd buffer which spans more than a page or the various
> xen_create_contiguous_region() requests.

You get a pretty clear BUG() message out if this happens. It actually 
tells you to enable 'swiotlb=force'!  The number of drivers that 
actually use multi-page buffers is really small -- we only fixed that 
case in 2.0 a few weeks ago.

I'd rather not waste 64MB of pre-reserved bounce buffers on 
small-memory systems that almost certainly don't need bounce buffers.

  -- Keir

Re: Re: [Patch] by default don't give all memory to dom0

[Mark Williamson]

> You get a pretty clear BUG() message out if this happens. It actually
> tells you to enable 'swiotlb=force'!  The number of drivers that
> actually use multi-page buffers is really small -- we only fixed that
> case in 2.0 a few weeks ago.

The USB drivers I've worked with tended to use multipage buffers, although 
only the mass storage driver seemed to use really large numbers of pages.

Cheers,
Mark

> I'd rather not waste 64MB of pre-reserved bounce buffers on
> small-memory systems that almost certainly don't need bounce buffers.
>
>   -- Keir
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Default memory unit

[David Hopwood]

Keir Fraser wrote:
> On 19 Aug 2005, at 01:13, Siddha, Suresh B wrote:
> 
>>> If Kbytes is to be the default unit, then that default should 
>>> probably be encoded in 'parse_size_and_unit', rather than having
>>> code like the above convert from bytes to Kbytes in many places.
>>
>> For "mem=" option, default unit is bytes. I think we should go with the
>> linux style and make the default unit as bytes. If no one has any 
>> objections, then I can send a patch for it.
> 
> Changing it will annoy people who are used to 2.0-style dom0_mem= 
> parameter. There is no good reason to change it: it's documented and, if 
> you want to avoid confusion, just stick a unit symbol on the end.

I disagree that there's no good reason to make the default unit consistent.
Currently Xen has a relatively small user community, so the time to change
it while annoying the fewest people is now.

-- 
David Hopwood <david.nospam.hopwood@blueyonder.co.uk>

Re: [Patch] by default don't give all memory to dom0

[Siddha, Suresh B]

On Fri, Aug 19, 2005 at 09:31:16AM +0100, Keir Fraser wrote:
> 
> On 19 Aug 2005, at 01:13, Siddha, Suresh B wrote:
> 
> >> If Kbytes is to be the default unit, then that default should 
> >> probably be
> >> encoded in 'parse_size_and_unit', rather than having code like the 
> >> above
> >> convert from bytes to Kbytes in many places.
> >
> > For "mem=" option, default unit is bytes. I think we should go with the
> > linux style and make the default unit as bytes. If no one has any 
> > objections,
> > then I can send a patch for it.
> 
> Changing it will annoy people who are used to 2.0-style dom0_mem= 
> parameter. There is no good reason to change it: it's documented and, 
> if you want to avoid confusion, just stick a unit symbol on the end.

And with the overnight changeset #6256, you changed the default behavior of 
"mem=" to kilobytes.

We had to change one or the other to achieve commonality.

thanks,
suresh

Re: [Patch] by default don't give all memory to dom0

[Siddha, Suresh B]

On Fri, Aug 19, 2005 at 09:44:03AM +0100, Keir Fraser wrote:
> 
> On 18 Aug 2005, at 20:24, Siddha, Suresh B wrote:
> 
> > By default, xen needs to reserve some portion of memory to satisfy
> > SWIOTLB and other contguous memory region requests. Following
> > the current swiotlb enabling mechanism, Appended patch reserves 128MB
> > of memory on systems with more than 2GB of RAM.
> 
> Hmmm... sounds reasonable. I'd rather have one dom0 memory parameter 
> though --- keeping dom0_mem but have +ve value mean 'allocate this 
> amount' and -ve mean 'allocate full memory - this amount'. Otherwise we 
> have two competing parameters specifying basically the same thing...
> 
> I don't much like hacky 'policies' that hardcode default reservations 
> in the hypervisor, but I think this one is pretty sensible.

I see this incorporated in changeset #6257. Thanks. 

> > Ideally shouldn't we enable SWIOTLB in dom0 and this DMA memory 
> > reservation
> > in hypervisor by default? Otherwise we will have a problem(even on 
> > systems
> > with less than 2GB of RAM) in servicing a driver DMA request to a
> > kmalloc'd buffer which spans more than a page or the various
> > xen_create_contiguous_region() requests.
> 
> You get a pretty clear BUG() message out if this happens. It actually 
> tells you to enable 'swiotlb=force'!  The number of drivers that 
> actually use multi-page buffers is really small -- we only fixed that 

If we decide not to support(/fix) those drivers, then we should enable
swiotlb only if the system has more than 4GB of RAM. Where is the current
2GB magic figure coming from?

> case in 2.0 a few weeks ago.
> 
> I'd rather not waste 64MB of pre-reserved bounce buffers on 
> small-memory systems that almost certainly don't need bounce buffers.

We can have a smaller swiotlb window (couple of MB?) on small-memory systems
if we want to support the drivers doing dma_map_single to multipage buffers.

thanks,
suresh