[PATCH 2/3] client: Add wl_display_soft_flush()
Pekka Paalanen
ppaalanen at gmail.com
Wed Jan 23 13:09:46 UTC 2019
On Fri, 7 Sep 2018 18:14:04 -0700
Lloyd Pique <lpique at google.com> wrote:
> This allows a client to occasionally flush the output buffers in the
> middle of making a large number of calls which fill them.
>
> A soft flush differs from a normal flush in that the buffer is not
> flushed if it is not full enough. The current criteria set is the buffer
> being half full.
>
> This does not completely replace the need to call wl_display_flush(),
> which ensures all the output is actually sent. However the client can
> make this call to properly handle any send errors that might occur.
>
> Includes a test to make sure that it can be used in a situation where a
> flush is needed and would return an EAGAIN, which would normally
> set a fatal display error if the core-client tried to do an internal
> flush if the output buffer became completely full.
>
> Signed-off-by: Lloyd Pique <lpique at google.com>
> ---
> src/connection.c | 30 ++++++++++++++-
> src/wayland-client-core.h | 3 ++
> src/wayland-client.c | 77 +++++++++++++++++++++++++++++----------
> src/wayland-private.h | 2 +-
> src/wayland-server.c | 6 +--
> tests/connection-test.c | 10 ++---
> tests/display-test.c | 62 +++++++++++++++++++++++++++++++
> tests/os-wrappers-test.c | 2 +-
> 8 files changed, 161 insertions(+), 31 deletions(-)
>
Hi Lloyd,
it seems the main justification for this patch is to avoid the overhead
of calling wl_display_flush() very often. Would you happen to have any
benchmark numbers on wl_display_flush() vs. wl_display_soft_flush()
from a real use case?
That would be very good to have in the commit message.
This patch does not compile:
/home/pq/git/wayland/src/connection.c: In function ‘wl_connection_write’:
/home/pq/git/wayland/src/connection.c:410:7: error: too few arguments to function ‘wl_connection_flush’
if (wl_connection_flush(connection) < 0)
/home/pq/git/wayland/src/connection.c: In function ‘wl_connection_put_fd’:
/home/pq/git/wayland/src/connection.c:460:7: error: too few arguments to function ‘wl_connection_flush’
if (wl_connection_flush(connection) < 0)
> diff --git a/src/connection.c b/src/connection.c
> index f965210..c271fa0 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -285,8 +285,31 @@ decode_cmsg(struct wl_buffer *buffer, struct msghdr *msg)
> return 0;
> }
>
> +static int
Always use bool for booleans, especially for predicates like this.
> +wl_buffer_is_half_full(struct wl_buffer *b)
> +{
> + return wl_buffer_size(b) > sizeof(b->data) / 2;
> +}
> +
> +static int
> +wl_fds_out_buffer_is_half_full(struct wl_connection* c)
wl_connection *c
> +{
> + return wl_buffer_size(&c->fds_out) > MAX_FDS_OUT * sizeof(int32_t) / 2;
> +}
> +
> +static int
> +connection_soft_flush_check(struct wl_connection* connection)
connection_soft_flush_needed? Then true/false return value is more
obvious.
> +{
> + /* For a soft flush, We use the the data buffer being half full, or the
> + * fds_out buffer being at half the message limit as the criteria for
> + * actually performing a flush. */
> +
> + return wl_buffer_is_half_full(&connection->out) ||
> + wl_fds_out_buffer_is_half_full(connection);
> +}
Block indent should be tabs, not spaces.
> +
> int
> -wl_connection_flush(struct wl_connection *connection)
> +wl_connection_flush(struct wl_connection *connection, int soft)
> {
> struct iovec iov[2];
> struct msghdr msg;
> @@ -297,6 +320,9 @@ wl_connection_flush(struct wl_connection *connection)
> if (!connection->want_flush)
> return 0;
>
> + if (soft && !connection_soft_flush_check(connection))
> + return 0;
> +
> tail = connection->out.tail;
> while (connection->out.head - connection->out.tail > 0) {
> wl_buffer_get_iov(&connection->out, iov, &count);
> @@ -400,7 +426,7 @@ wl_connection_queue(struct wl_connection *connection,
> if (connection->out.head - connection->out.tail +
> count > ARRAY_LENGTH(connection->out.data)) {
> connection->want_flush = 1;
> - if (wl_connection_flush(connection) < 0)
> + if (wl_connection_flush(connection, 0) < 0)
> return -1;
> }
>
> diff --git a/src/wayland-client-core.h b/src/wayland-client-core.h
> index 03e781b..84b3b41 100644
> --- a/src/wayland-client-core.h
> +++ b/src/wayland-client-core.h
> @@ -234,6 +234,9 @@ wl_display_get_protocol_error(struct wl_display *display,
> int
> wl_display_flush(struct wl_display *display);
>
> +int
> +wl_display_soft_flush(struct wl_display *display);
> +
> int
> wl_display_roundtrip_queue(struct wl_display *display,
> struct wl_event_queue *queue);
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index 29ae1e0..eea634d 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -1957,6 +1957,31 @@ wl_display_get_protocol_error(struct wl_display *display,
> return ret;
> }
>
> +static int
> +display_flush(struct wl_display *display, int soft)
> +{
> + int ret;
> +
> + pthread_mutex_lock(&display->mutex);
> +
> + if (display->last_error) {
> + errno = display->last_error;
> + ret = -1;
> + } else {
> + /* We don't make EPIPE a fatal error here, so that we may try to
> + * read events after the failed flush. When the compositor sends
> + * an error it will close the socket, and if we make EPIPE fatal
> + * here we don't get a chance to process the error. */
> + ret = wl_connection_flush(display->connection, soft);
> + if (ret < 0 && errno != EAGAIN && errno != EPIPE)
> + display_fatal_error(display, errno);
> + }
> +
> + pthread_mutex_unlock(&display->mutex);
> +
> + return ret;
> +}
> +
>
> /** Send all buffered requests on the display to the server
> *
> @@ -1978,26 +2003,40 @@ wl_display_get_protocol_error(struct wl_display *display,
> WL_EXPORT int
> wl_display_flush(struct wl_display *display)
> {
> - int ret;
> -
> - pthread_mutex_lock(&display->mutex);
> -
> - if (display->last_error) {
> - errno = display->last_error;
> - ret = -1;
> - } else {
> - /* We don't make EPIPE a fatal error here, so that we may try to
> - * read events after the failed flush. When the compositor sends
> - * an error it will close the socket, and if we make EPIPE fatal
> - * here we don't get a chance to process the error. */
> - ret = wl_connection_flush(display->connection);
> - if (ret < 0 && errno != EAGAIN && errno != EPIPE)
> - display_fatal_error(display, errno);
> - }
> -
> - pthread_mutex_unlock(&display->mutex);
> + return display_flush(display, 0);
> +}
>
> - return ret;
> +/** Performs a wl_display_flush() only if the buffer is getting full
> + *
> + * \param display The display context object
> + * \return The number of bytes sent on success, or -1 on failure.
> + *
> + * Performs a conditional flush of buffered data to the server. The data is
> + * only actually flushed if a significant amount of data has been buffered. If
> + * a flush is performed, this function is is equivalent to calling
> + * wl_display_flush(). If a flush is not needed, this function just returns
> + * zero without performing a flush.
> + *
> + * Clients should call this function occasionally while making a large number
> + * of message output calls, where there is a danger of the output buffer
> + * being filled. If the buffers do get completely full, the library will
> + * try to do an internal flush, but if an error occurs the only option
> + * is to set a fatal display error.
I think this paragraph could be worded better, and it forgets to
explain why not simply call wl_display_flush() instead.
"A failure during an automatic internal flush as part of emitting a
request would be a fatal display error causing a disconnect. If a
client is sending a large volume of requests without ensuring the
Wayland connection remains writable, it can use wl_display_soft_flush()
to test if the connection is becoming congested.
wl_display_soft_flush() does not have the overhead of the forced flush
in wl_display_flush()."
Maybe something like that?
> + *
> + * Clients should also check for a return value of -1, check errno, and respond
> + * appropriately, such as polling on the display file descriptor on EAGAIN.
> + *
> + * Clients should still call wl_display_flush() to ensure all output is
> + * completely flushed before blocking on input from the display fd.
> + *
> + * wl_display_soft_flush() does not block.
Aren't these three paragraphs redundant with the reference to
wl_display_flush()? They also do not duplicate all the notes about
wl_display_flush() so they seem either unnecessary or incomplete.
Well, the middle paragraph is a good one to keep.
> + *
> + * \memberof wl_display
We really should get a habit of adding a note on which version a new
function appeared. :-)
This will most likely become 1.17 release, but I think the version
number to use is the current one: 1.16.90. That way the alpha release
1.16.91 will have a version number fulfilling this "requirement" and
people can simply require 1.16.90.
> + */
> +WL_EXPORT int
> +wl_display_soft_flush(struct wl_display *display)
> +{
> + return display_flush(display, 1);
> }
>
> /** Set the user data associated with a proxy
> diff --git a/src/wayland-private.h b/src/wayland-private.h
> index 29516ec..ba183fc 100644
> --- a/src/wayland-private.h
> +++ b/src/wayland-private.h
> @@ -121,7 +121,7 @@ void
> wl_connection_consume(struct wl_connection *connection, size_t size);
>
> int
> -wl_connection_flush(struct wl_connection *connection);
> +wl_connection_flush(struct wl_connection *connection, int soft);
Hmm, I see that is what you did with display_flush(), and it is
very convenient to just pass the flag down to wl_connection_flush().
The flag should be named though, to make the code read better. Perhaps
instead of 'int soft' or 'bool soft', use 'uint32_t flags' and define
the "soft" flag as an enum?
That way we get calls like wl_connection_flush(conn,
WL_FLUSH_FLAG_SOFT) or such which reads much better than
wl_connection_flush(conn, true).
Or instead of 'uint32_t flags', an 'enum wl_flush_mode' with items
WL_FLUSH_MODE_HARD and WL_FLUSH_MODE_SOFT or whatever you think are
descriptive names.
Updating all the existing wl_connection_flush() callers into the new
form of a hard flush should ideally be a separate patch as well, since
it's a number of lines and without any intended change in behaviour. It
would help reviewers.
>
> uint32_t
> wl_connection_pending_input(struct wl_connection *connection);
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index eae8d2e..43e4099 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -332,7 +332,7 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
> }
>
> if (mask & WL_EVENT_WRITABLE) {
> - len = wl_connection_flush(connection);
> + len = wl_connection_flush(connection, 0);
> if (len < 0 && errno != EAGAIN) {
> destroy_client_with_error(
> client, "failed to flush client connection");
> @@ -454,7 +454,7 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
> WL_EXPORT void
> wl_client_flush(struct wl_client *client)
> {
> - wl_connection_flush(client->connection);
> + wl_connection_flush(client->connection, 0);
> }
>
> /** Get the display object for the given client
> @@ -1268,7 +1268,7 @@ wl_display_flush_clients(struct wl_display *display)
> int ret;
>
> wl_list_for_each_safe(client, next, &display->client_list, link) {
> - ret = wl_connection_flush(client->connection);
> + ret = wl_connection_flush(client->connection, 0);
> if (ret < 0 && errno == EAGAIN) {
> wl_event_source_fd_update(client->source,
> WL_EVENT_WRITABLE |
> diff --git a/tests/connection-test.c b/tests/connection-test.c
> index 018e2ac..4248f4a 100644
> --- a/tests/connection-test.c
> +++ b/tests/connection-test.c
> @@ -76,7 +76,7 @@ TEST(connection_write)
> connection = setup(s);
>
> assert(wl_connection_write(connection, message, sizeof message) == 0);
> - assert(wl_connection_flush(connection) == sizeof message);
> + assert(wl_connection_flush(connection, 0) == sizeof message);
> assert(read(s[1], buffer, sizeof buffer) == sizeof message);
> assert(memcmp(message, buffer, sizeof message) == 0);
>
> @@ -118,9 +118,9 @@ TEST(connection_queue)
> * we receive the two messages on the other fd. */
>
> assert(wl_connection_queue(connection, message, sizeof message) == 0);
> - assert(wl_connection_flush(connection) == 0);
> + assert(wl_connection_flush(connection, 0) == 0);
> assert(wl_connection_write(connection, message, sizeof message) == 0);
> - assert(wl_connection_flush(connection) == 2 * sizeof message);
> + assert(wl_connection_flush(connection, 0) == 2 * sizeof message);
> assert(read(s[1], buffer, sizeof buffer) == 2 * sizeof message);
> assert(memcmp(message, buffer, sizeof message) == 0);
> assert(memcmp(message, buffer + sizeof message, sizeof message) == 0);
> @@ -212,7 +212,7 @@ marshal(struct marshal_data *data, const char *format, int size, ...)
> assert(closure);
> assert(wl_closure_send(closure, data->write_connection) == 0);
> wl_closure_destroy(closure);
> - assert(wl_connection_flush(data->write_connection) == size);
> + assert(wl_connection_flush(data->write_connection, 0) == size);
> assert(read(data->s[0], data->buffer, sizeof data->buffer) == size);
>
> assert(data->buffer[0] == sender.id);
> @@ -476,7 +476,7 @@ marshal_demarshal(struct marshal_data *data,
> assert(closure);
> assert(wl_closure_send(closure, data->write_connection) == 0);
> wl_closure_destroy(closure);
> - assert(wl_connection_flush(data->write_connection) == size);
> + assert(wl_connection_flush(data->write_connection, 0) == size);
>
> assert(wl_connection_read(data->read_connection) == size);
>
> diff --git a/tests/display-test.c b/tests/display-test.c
> index 23f0635..00eda24 100644
> --- a/tests/display-test.c
> +++ b/tests/display-test.c
> @@ -1392,3 +1392,65 @@ TEST(client_send_error_tst)
>
> display_destroy(d);
> }
> +
> +static void
> +display_soft_flush_and_poll(struct wl_display *display, int *eagain_count)
> +{
> + struct pollfd pfd;
> +
> + /* If a flush is recommended, we should perform one */
> + while (wl_display_soft_flush(display) == -1) {
> + /* For this test, we only expect an EAGAIN error */
> + assert(errno == EAGAIN);
How do you ensure the flush actually fails, what will keep the server
busy long enough?
In the first patch, you had a sleep in the server to ensure the client
will overflow the connection, but this looks like it would just depend
on luck. Well, sleeps are luck too, but even more luck here than there.
> +
> + /* Track the count of EAGAIN */
> + *eagain_count += 1;
> +
> + /* Wait for the display to be ready for more output. */
> + pfd.fd = wl_display_get_fd(display);
> + pfd.events = POLLOUT;
> + assert(poll(&pfd, 1, -1) == 1);
> + }
> +}
> +
> +static void
> +soft_flush_client(void)
> +{
> + int eagain_count = 0;
> + struct client *c = client_connect();
> +
> + /* Verify we do not have any errors at this point. */
> + assert(wl_display_get_error(c->wl_display) == 0);
> +
> + /* A soft flush should not write anything.*/
> + assert(wl_display_soft_flush(c->wl_display) == 0);
> +
> + /* Fill up the client and server buffers With the default error
> + * handling, this will abort the client. */
> + fill_client_send_buffers(c->wl_display, display_soft_flush_and_poll,
> + &eagain_count);
> +
> + /* Verify we do not have any errors at this point. */
> + assert(wl_display_get_error(c->wl_display) == 0);
> +
> + /* A soft flush should not write anything.*/
> + assert(wl_display_soft_flush(c->wl_display) == 0);
> +
> + /* .. but we should have had to wait on at least one flush. */
> + assert(eagain_count > 0);
Your tests are well documented, that's nice.
> +
> + client_disconnect_nocheck(c);
> +}
> +
> +TEST(soft_flush_tst)
> +{
> + struct display *d = display_create();
> +
> + test_set_timeout(2);
> +
> + client_create_noarg(d, soft_flush_client);
> +
> + display_run(d);
> +
> + display_destroy(d);
> +}
> diff --git a/tests/os-wrappers-test.c b/tests/os-wrappers-test.c
> index 102622c..141501d 100644
> --- a/tests/os-wrappers-test.c
> +++ b/tests/os-wrappers-test.c
> @@ -248,7 +248,7 @@ marshal_demarshal(struct marshal_data *data,
> assert(closure);
> assert(wl_closure_send(closure, data->write_connection) == 0);
> wl_closure_destroy(closure);
> - assert(wl_connection_flush(data->write_connection) == size);
> + assert(wl_connection_flush(data->write_connection, 0) == size);
>
> assert(wl_connection_read(data->read_connection) == size);
>
The logic at large in this patch looks good.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20190123/b70d0dc8/attachment.sig>
More information about the wayland-devel
mailing list