[LEDE-DEV] [PATCH 2/3] ubusd: make `tx_queue` backlog dynamic

[Alexandru Ardelean]

On Wed, Jun 7, 2017 at 2:09 PM, Alexandru Ardelean
<ardeleanalex at gmail.com> wrote:
> It's not very often that the tx_queue is used,
> to store backlog messages to send to a client.
>
> And for most cases, 32 backlog messages seems to be enough.
> In fact, for most cases, I've seen ~1 entry in the queue
> being used every now-n-then.
>
> The issue is more visible/present with the `ubus list` command.
> I've traced the code to ubusd_handle_lookup() in:
>
> ```
>     if (!attr[UBUS_ATTR_OBJPATH]) {
>         avl_for_each_element(&path, obj, path)
>             ubusd_send_obj(cl, ub, obj);
>         return 0;
>     }
> ```
> The code-path eventually leads to `ubus_msg_send()`.
> It seems that once the first element is queued, then
> the condition check for `(!cl->tx_queue[cl->txq_cur])`
> will queue all messages iterated in the above snippet,
> without trying any writes.
>
> This can be solved, either by making the queue dynamic
> and allow it to expand above the current fixed limit (1).
>
> Or, by forcing/allowing writes during the tx_queue-ing (2).
>
> This patch implements (1).

To add to this.
I also implemented (2).
Link:
https://github.com/commodo/ubus/commit/78ac089e3664f3b0422271c84283f754119d4f4d

However, it does not seem to help with the issue.
I.e. the lua script in the previous patch, manages to fill up the
tx_queue even with
trying to send data from the tx_queue [ while queue-ing ].
Tho, I do feel that the patch (from Github) is forcing things a bit.

>
> Signed-off-by: Alexandru Ardelean <ardeleanalex at gmail.com>
> ---
>  ubusd.c       | 18 +++++++++---------
>  ubusd.h       |  6 +++---
>  ubusd_proto.c |  1 +
>  3 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/ubusd.c b/ubusd.c
> index f060b38..8fdb85b 100644
> --- a/ubusd.c
> +++ b/ubusd.c
> @@ -59,6 +59,7 @@ struct ubus_msg_buf *ubus_msg_new(void *data, int len, bool shared)
>                 return NULL;
>
>         ub->fd = -1;
> +       INIT_LIST_HEAD(&ub->list);
>
>         if (shared) {
>                 ub->refcount = ~0;
> @@ -138,11 +139,9 @@ static int ubus_msg_writev(int fd, struct ubus_msg_buf *ub, int offset)
>
>  static void ubus_msg_enqueue(struct ubus_client *cl, struct ubus_msg_buf *ub)
>  {
> -       if (cl->tx_queue[cl->txq_tail])
> -               return;
> -
> -       cl->tx_queue[cl->txq_tail] = ubus_msg_ref(ub);
> -       cl->txq_tail = (cl->txq_tail + 1) % ARRAY_SIZE(cl->tx_queue);
> +       ub = ubus_msg_ref(ub);
> +       if (ub)
> +               list_add_tail(&ub->list, &cl->tx_queue);
>  }
>
>  /* takes the msgbuf reference */
> @@ -153,7 +152,7 @@ void ubus_msg_send(struct ubus_client *cl, struct ubus_msg_buf *ub, bool free)
>         if (ub->hdr.type != UBUS_MSG_MONITOR)
>                 ubusd_monitor_message(cl, ub, true);
>
> -       if (!cl->tx_queue[cl->txq_cur]) {
> +       if (list_empty(&cl->tx_queue)) {
>                 written = ubus_msg_writev(cl->sock.fd, ub, 0);
>
>                 if (written < 0)
> @@ -176,7 +175,9 @@ out:
>
>  static struct ubus_msg_buf *ubus_msg_head(struct ubus_client *cl)
>  {
> -       return cl->tx_queue[cl->txq_cur];
> +       if (list_empty(&cl->tx_queue))
> +               return NULL;
> +       return list_first_entry(&cl->tx_queue, struct ubus_msg_buf, list);
>  }
>
>  static void ubus_msg_dequeue(struct ubus_client *cl)
> @@ -186,10 +187,9 @@ static void ubus_msg_dequeue(struct ubus_client *cl)
>         if (!ub)
>                 return;
>
> +       list_del(&ub->list);
>         ubus_msg_free(ub);
>         cl->txq_ofs = 0;
> -       cl->tx_queue[cl->txq_cur] = NULL;
> -       cl->txq_cur = (cl->txq_cur + 1) % ARRAY_SIZE(cl->tx_queue);
>  }
>
>  static void handle_client_disconnect(struct ubus_client *cl)
> diff --git a/ubusd.h b/ubusd.h
> index 5031ed4..6748c65 100644
> --- a/ubusd.h
> +++ b/ubusd.h
> @@ -23,12 +23,12 @@
>  #include "ubusmsg.h"
>  #include "ubusd_acl.h"
>
> -#define UBUSD_CLIENT_BACKLOG   32
>  #define UBUS_OBJ_HASH_BITS     4
>
>  extern struct blob_buf b;
>
>  struct ubus_msg_buf {
> +       struct list_head list;
>         uint32_t refcount; /* ~0: uses external data buffer */
>         struct ubus_msghdr hdr;
>         struct blob_attr *data;
> @@ -47,8 +47,8 @@ struct ubus_client {
>
>         struct list_head objects;
>
> -       struct ubus_msg_buf *tx_queue[UBUSD_CLIENT_BACKLOG];
> -       unsigned int txq_cur, txq_tail, txq_ofs;
> +       unsigned int txq_ofs;
> +       struct list_head tx_queue;
>
>         struct ubus_msg_buf *pending_msg;
>         int pending_msg_offset;
> diff --git a/ubusd_proto.c b/ubusd_proto.c
> index 72da7a7..631047b 100644
> --- a/ubusd_proto.c
> +++ b/ubusd_proto.c
> @@ -480,6 +480,7 @@ struct ubus_client *ubusd_proto_new_client(int fd, uloop_fd_handler cb)
>                 goto free;
>
>         INIT_LIST_HEAD(&cl->objects);
> +       INIT_LIST_HEAD(&cl->tx_queue);
>         cl->sock.fd = fd;
>         cl->sock.cb = cb;
>         cl->pending_msg_fd = -1;
> --
> 2.7.4
>