Re: [RFC xserver 1/1] xwayland: reduce over-damage

[Pekka Paalanen]

On Mon, 22 Jan 2018 13:14:10 -0500
Adam Jackson  wrote:

> On Mon, 2018-01-22 at 14:51 +, Daniel Stone wrote:
> >  Hi Pekka,
> > 
> > On 20 December 2017 at 11:18, Pekka Paalanen  wrote:  
> > > If an X11 app draws a little here, some there, and a tiny bit in the
> > > opposite corner, using RegionExtents for the damage to be sent to the
> > > Wayland compositor will cause massive over-damaging.
> > > 
> > > However, we cannot blindly send an arbitrary number of damage
> > > rectangles, because there is a risk of overflowing the Wayland
> > > connection. If that happens, it triggers an abort in libwayland-client.
> > > 
> > > Try to be more accurate with the damage by sending up to 500 rectangles
> > > per window, and fall back to extents otherwise. The number is completely
> > > arbitrary.  
> > 
> > I might have said this on IRC, but 500 sails close enough to our
> > request limit for comfort. I can't find the mail where I did the
> > maths, but ISTR for damage requests, it's around the 6000-7000 mark.
> > Pulling 256 as an equally arbitrary number out of the air (arguably
> > even that number of TexSubImage requests represents a DoS), this is:
> > Reviewed-by: Daniel Stone   
> 
> Changed the magic number to 256 and merged, thanks:
> 
> remote: E: failed to find patch for rev 
> f72587ecc7e1dedfb20a999a0e600b83c06a1b29.
> remote: I: 0 patch(es) updated to state Accepted.
> To ssh://git.freedesktop.org/git/xorg/xserver
>a5e9bcad7..f72587ecc  master -> master

Thank you both!

Daniel, a damage request is 8 + 4 * 4 = 24 B. 500 requests is 12000 B,
256 requests is 6144 B. The libwayland internal send buffer is 4096 B,
so for both values we are relying on the kernel socket buffer already.
Furthermore, the limit is per window and number of windows is
effectively unlimited. Xwayland loops over windows without a
possibility to poll for writable.


On Mon, 22 Jan 2018 14:56:12 +0100
Olivier Fourdan  wrote:

> Hi Pekka,
> 
> On 22 January 2018 at 14:34, Pekka Paalanen 
> wrote:

> > should I invest the effort on a better algorithm than in this patch or
> > would this be acceptable for landing?
> >  
> 
> Not sure which better algorithm you might have in mind here, but the
> approach taken in your patch seems simple enough, do we have a rough idea
> of the average number of rectangles we usually deal with here?

The two other approaches in the cover letter:
https://lists.x.org/archives/xorg-devel/2017-December/055474.html
Of course, the optimization approach could use various different
methods. There is also an implementation detail: pixman_region*_t uses
a banded format which increases the number of rectangles compared to a
free format, both using non-overlapping rectangles. The Wayland
protocol does not require the banded format.

I have no statistics about the rectangles on any general use case.

In summary, the Wayland connection overflowing issue would need a
specific solution. Sending optimized damage rectangles may help making
the overflow less likely, but it will never eliminate the possibility.
This patch is about a performance improvement, which is mostly
orthogonal to avoiding the overflow.


Thanks,
pq


pgptHBZPef5Rq.pgp
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC xserver 1/1] xwayland: reduce over-damage

[Adam Jackson]

On Mon, 2018-01-22 at 14:51 +, Daniel Stone wrote:
>  Hi Pekka,
> 
> On 20 December 2017 at 11:18, Pekka Paalanen  wrote:
> > If an X11 app draws a little here, some there, and a tiny bit in the
> > opposite corner, using RegionExtents for the damage to be sent to the
> > Wayland compositor will cause massive over-damaging.
> > 
> > However, we cannot blindly send an arbitrary number of damage
> > rectangles, because there is a risk of overflowing the Wayland
> > connection. If that happens, it triggers an abort in libwayland-client.
> > 
> > Try to be more accurate with the damage by sending up to 500 rectangles
> > per window, and fall back to extents otherwise. The number is completely
> > arbitrary.
> 
> I might have said this on IRC, but 500 sails close enough to our
> request limit for comfort. I can't find the mail where I did the
> maths, but ISTR for damage requests, it's around the 6000-7000 mark.
> Pulling 256 as an equally arbitrary number out of the air (arguably
> even that number of TexSubImage requests represents a DoS), this is:
> Reviewed-by: Daniel Stone 

Changed the magic number to 256 and merged, thanks:

remote: E: failed to find patch for rev 
f72587ecc7e1dedfb20a999a0e600b83c06a1b29.
remote: I: 0 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   a5e9bcad7..f72587ecc  master -> master

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC xserver 1/1] xwayland: reduce over-damage

[Daniel Stone]

 Hi Pekka,

On 20 December 2017 at 11:18, Pekka Paalanen  wrote:
> If an X11 app draws a little here, some there, and a tiny bit in the
> opposite corner, using RegionExtents for the damage to be sent to the
> Wayland compositor will cause massive over-damaging.
>
> However, we cannot blindly send an arbitrary number of damage
> rectangles, because there is a risk of overflowing the Wayland
> connection. If that happens, it triggers an abort in libwayland-client.
>
> Try to be more accurate with the damage by sending up to 500 rectangles
> per window, and fall back to extents otherwise. The number is completely
> arbitrary.

I might have said this on IRC, but 500 sails close enough to our
request limit for comfort. I can't find the mail where I did the
maths, but ISTR for damage requests, it's around the 6000-7000 mark.
Pulling 256 as an equally arbitrary number out of the air (arguably
even that number of TexSubImage requests represents a DoS), this is:
Reviewed-by: Daniel Stone 

Cheers,
Daniel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC xserver 1/1] xwayland: reduce over-damage

[Olivier Fourdan]

Hi Pekka,

On 22 January 2018 at 14:34, Pekka Paalanen 
wrote:

> On Wed, 20 Dec 2017 13:18:45 +0200
> Pekka Paalanen  wrote:
>
> > From: Pekka Paalanen 
> >
> > If an X11 app draws a little here, some there, and a tiny bit in the
> > opposite corner, using RegionExtents for the damage to be sent to the
> > Wayland compositor will cause massive over-damaging.
> >
> > However, we cannot blindly send an arbitrary number of damage
> > rectangles, because there is a risk of overflowing the Wayland
> > connection. If that happens, it triggers an abort in libwayland-client.
> >
> > Try to be more accurate with the damage by sending up to 500 rectangles
> > per window, and fall back to extents otherwise. The number is completely
> > arbitrary.
> >
> > Signed-off-by: Pekka Paalanen 
> > ---
> >  hw/xwayland/xwayland.c | 18 +++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
> > index c5a3ae7ae..383680d7a 100644
> > --- a/hw/xwayland/xwayland.c
> > +++ b/hw/xwayland/xwayland.c
> > @@ -629,6 +629,7 @@ xwl_window_post_damage(struct xwl_window *xwl_window)
> >  BoxPtr box;
> >  struct wl_buffer *buffer;
> >  PixmapPtr pixmap;
> > +int i;
> >
> >  assert(!xwl_window->frame_callback);
> >
> > @@ -644,9 +645,20 @@ xwl_window_post_damage(struct xwl_window
> *xwl_window)
> >
> >  wl_surface_attach(xwl_window->surface, buffer, 0, 0);
> >
> > -box = RegionExtents(region);
> > -wl_surface_damage(xwl_window->surface, box->x1, box->y1,
> > -box->x2 - box->x1, box->y2 - box->y1);
> > +/* Arbitrary limit to try to avoid flooding the Wayland
> > + * connection. If we flood it too much anyway, this could
> > + * abort in libwayland-client.
> > + */
> > +if (RegionNumRects(region) > 500) {
> > +box = RegionExtents(region);
> > +wl_surface_damage(xwl_window->surface, box->x1, box->y1,
> > +  box->x2 - box->x1, box->y2 - box->y1);
> > +} else {
> > +box = RegionRects(region);
> > +for (i = 0; i < RegionNumRects(region); i++, box++)
> > +wl_surface_damage(xwl_window->surface, box->x1, box->y1,
> > +  box->x2 - box->x1, box->y2 - box->y1);
> > +}
> >
> >  xwl_window->frame_callback = wl_surface_frame(xwl_window->surface);
> >  wl_callback_add_listener(xwl_window->frame_callback,
> _listener, xwl_window);
>
> Hi,
>
> should I invest the effort on a better algorithm than in this patch or
> would this be acceptable for landing?
>

Not sure which better algorithm you might have in mind here, but the
approach taken in your patch seems simple enough, do we have a rough idea
of the average number of rectangles we usually deal with here?

Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC xserver 1/1] xwayland: reduce over-damage

[Pekka Paalanen]

On Wed, 20 Dec 2017 13:18:45 +0200
Pekka Paalanen  wrote:

> From: Pekka Paalanen 
> 
> If an X11 app draws a little here, some there, and a tiny bit in the
> opposite corner, using RegionExtents for the damage to be sent to the
> Wayland compositor will cause massive over-damaging.
> 
> However, we cannot blindly send an arbitrary number of damage
> rectangles, because there is a risk of overflowing the Wayland
> connection. If that happens, it triggers an abort in libwayland-client.
> 
> Try to be more accurate with the damage by sending up to 500 rectangles
> per window, and fall back to extents otherwise. The number is completely
> arbitrary.
> 
> Signed-off-by: Pekka Paalanen 
> ---
>  hw/xwayland/xwayland.c | 18 +++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
> index c5a3ae7ae..383680d7a 100644
> --- a/hw/xwayland/xwayland.c
> +++ b/hw/xwayland/xwayland.c
> @@ -629,6 +629,7 @@ xwl_window_post_damage(struct xwl_window *xwl_window)
>  BoxPtr box;
>  struct wl_buffer *buffer;
>  PixmapPtr pixmap;
> +int i;
>  
>  assert(!xwl_window->frame_callback);
>  
> @@ -644,9 +645,20 @@ xwl_window_post_damage(struct xwl_window *xwl_window)
>  
>  wl_surface_attach(xwl_window->surface, buffer, 0, 0);
>  
> -box = RegionExtents(region);
> -wl_surface_damage(xwl_window->surface, box->x1, box->y1,
> -box->x2 - box->x1, box->y2 - box->y1);
> +/* Arbitrary limit to try to avoid flooding the Wayland
> + * connection. If we flood it too much anyway, this could
> + * abort in libwayland-client.
> + */
> +if (RegionNumRects(region) > 500) {
> +box = RegionExtents(region);
> +wl_surface_damage(xwl_window->surface, box->x1, box->y1,
> +  box->x2 - box->x1, box->y2 - box->y1);
> +} else {
> +box = RegionRects(region);
> +for (i = 0; i < RegionNumRects(region); i++, box++)
> +wl_surface_damage(xwl_window->surface, box->x1, box->y1,
> +  box->x2 - box->x1, box->y2 - box->y1);
> +}
>  
>  xwl_window->frame_callback = wl_surface_frame(xwl_window->surface);
>  wl_callback_add_listener(xwl_window->frame_callback, _listener, 
> xwl_window);

Hi,

should I invest the effort on a better algorithm than in this patch or
would this be acceptable for landing?


Thanks,
pq
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[RFC xserver 1/1] xwayland: reduce over-damage

[Pekka Paalanen]

From: Pekka Paalanen 

If an X11 app draws a little here, some there, and a tiny bit in the
opposite corner, using RegionExtents for the damage to be sent to the
Wayland compositor will cause massive over-damaging.

However, we cannot blindly send an arbitrary number of damage
rectangles, because there is a risk of overflowing the Wayland
connection. If that happens, it triggers an abort in libwayland-client.

Try to be more accurate with the damage by sending up to 500 rectangles
per window, and fall back to extents otherwise. The number is completely
arbitrary.

Signed-off-by: Pekka Paalanen 
---
 hw/xwayland/xwayland.c | 18 +++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index c5a3ae7ae..383680d7a 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -629,6 +629,7 @@ xwl_window_post_damage(struct xwl_window *xwl_window)
 BoxPtr box;
 struct wl_buffer *buffer;
 PixmapPtr pixmap;
+int i;
 
 assert(!xwl_window->frame_callback);
 
@@ -644,9 +645,20 @@ xwl_window_post_damage(struct xwl_window *xwl_window)
 
 wl_surface_attach(xwl_window->surface, buffer, 0, 0);
 
-box = RegionExtents(region);
-wl_surface_damage(xwl_window->surface, box->x1, box->y1,
-box->x2 - box->x1, box->y2 - box->y1);
+/* Arbitrary limit to try to avoid flooding the Wayland
+ * connection. If we flood it too much anyway, this could
+ * abort in libwayland-client.
+ */
+if (RegionNumRects(region) > 500) {
+box = RegionExtents(region);
+wl_surface_damage(xwl_window->surface, box->x1, box->y1,
+  box->x2 - box->x1, box->y2 - box->y1);
+} else {
+box = RegionRects(region);
+for (i = 0; i < RegionNumRects(region); i++, box++)
+wl_surface_damage(xwl_window->surface, box->x1, box->y1,
+  box->x2 - box->x1, box->y2 - box->y1);
+}
 
 xwl_window->frame_callback = wl_surface_frame(xwl_window->surface);
 wl_callback_add_listener(xwl_window->frame_callback, _listener, 
xwl_window);
-- 
2.13.6

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel