Xen devel - Aug 2005 - [Patch] by default don''t give all memory to dom0

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel