[LEDE-DEV] [PATCH] ubusd: Use linked list for queued messages (alt.)

[Benjamin Hansmann]

Hi,

What is the acceptance state of this patch? In patchwork it has the
status `superseded'. Does it mean that the issue has been or will be
solved by other means?

Thanks,

Benjamin


On Thu, 2018-05-03 at 12:06 +0200, Benjamin Hansmann wrote:
> The fixed size array for queuing messages led to discarding messages
> when it was full, using a linked list instead solves this issue.
> 
> The motivation was that for a recursive "ubus list" the function
> ubusd_proto.c:ubusd_handle_lookup() produces more than n messages in
> one uloop cycle when n objects are registered on the bus.
> 
> Signed-off-by: Benjamin Hansmann <i at qbox.audio>
> ---
>  ubusd.c       | 29 +++++++++++++++++------------
>  ubusd.h       | 10 +++++++---
>  ubusd_proto.c |  1 +
>  3 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/ubusd.c b/ubusd.c
> index ba1ff07..e58624a 100644
> --- a/ubusd.c
> +++ b/ubusd.c
> @@ -138,11 +138,11 @@ 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])
> +	struct ubus_msg_qentry *entry = calloc(1, sizeof(*entry));
> +	if (!entry)
>  		return;
> -
> -	cl->tx_queue[cl->txq_tail] = ubus_msg_ref(ub);
> -	cl->txq_tail = (cl->txq_tail + 1) % ARRAY_SIZE(cl-
> >tx_queue);
> +	entry->msg = ubus_msg_ref(ub);
> +	list_add_tail(&entry->queue, &cl->tx_queue);
>  }
>  
>  /* takes the msgbuf reference */
> @@ -153,7 +153,7 @@ void ubus_msg_send(struct ubus_client *cl, struct
> ubus_msg_buf *ub)
>  	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)
> @@ -172,20 +172,25 @@ void ubus_msg_send(struct ubus_client *cl,
> struct ubus_msg_buf *ub)
>  
>  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;
> +	struct ubus_msg_qentry *entry;
> +	entry = list_first_entry(&cl->tx_queue, struct
> ubus_msg_qentry, queue);
> +	return entry->msg;
>  }
>  
>  static void ubus_msg_dequeue(struct ubus_client *cl)
>  {
> -	struct ubus_msg_buf *ub = ubus_msg_head(cl);
> -
> -	if (!ub)
> +	if (list_empty(&cl->tx_queue))
>  		return;
>  
> -	ubus_msg_free(ub);
> +	struct ubus_msg_qentry *entry;
> +	entry = list_first_entry(&cl->tx_queue, struct
> ubus_msg_qentry, queue);
> +
> +	list_del(&entry->queue);
>  	cl->txq_ofs = 0;
> -	cl->tx_queue[cl->txq_cur] = NULL;
> -	cl->txq_cur = (cl->txq_cur + 1) % ARRAY_SIZE(cl->tx_queue);
> +	ubus_msg_free(entry->msg);
> +	free(entry);
>  }
>  
>  static void handle_client_disconnect(struct ubus_client *cl)
> diff --git a/ubusd.h b/ubusd.h
> index 4d87920..8758916 100644
> --- a/ubusd.h
> +++ b/ubusd.h
> @@ -23,7 +23,6 @@
>  #include "ubusmsg.h"
>  #include "ubusd_acl.h"
>  
> -#define UBUSD_CLIENT_BACKLOG	32
>  #define UBUS_OBJ_HASH_BITS	4
>  
>  extern struct blob_buf b;
> @@ -36,6 +35,11 @@ struct ubus_msg_buf {
>  	int len;
>  };
>  
> +struct ubus_msg_qentry {
> +	struct list_head queue;
> +	struct ubus_msg_buf *msg;
> +};
> +
>  struct ubus_client {
>  	struct ubus_id id;
>  	struct uloop_fd sock;
> @@ -48,8 +52,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;
> +	struct list_head tx_queue;
> +	unsigned int txq_ofs;
>  
>  	struct ubus_msg_buf *pending_msg;
>  	struct ubus_msg_buf *retmsg;
> diff --git a/ubusd_proto.c b/ubusd_proto.c
> index 2d04b5a..ac9d075 100644
> --- a/ubusd_proto.c
> +++ b/ubusd_proto.c
> @@ -495,6 +495,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;