On Wed, May 2, 2018 at 11:55 PM, Benjamin Hansmann <i@qbox.audio> wrote: > The fixed size array for queuing messages led to discarding messages > when it was full, using a linked list instead solves this issue. > > Having the list_head link in the ubus_msg_buf itself avoids the > allocation of more memory for an independent list. > > 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. >
Hey, I second this patch. I also proposed it a while back http://patchwork.ozlabs.org/patch/772366/ This was part of a series of ubus fixes. I added a test that shows the issue [I was seeing]: http://patchwork.ozlabs.org/patch/772365/ The issue [I was seeing] was]more of a fast-producer - slow-consumer issue. This was caused mostly when running a ubus cmd on the TTY serial [ causing a slowdown which showed the issue ]. But in my case, another patch had a greater impact that the dynamic TX queue ; this one: http://patchwork.ozlabs.org/patch/772364/ I was never hitting the 32 entries limit. I also discarded this patch [at the time] because I was unsure whether it causes more issues [than it solves]. And did not have time to go more in-depth with it. But I think, that if this helps your case, it should be good. Also [with this occasion]: thanks Felix for merging my other ubus patches. Thanks Alex > Signed-off-by: Benjamin Hansmann <i@qbox.audio> > --- > ubusd.c | 19 ++++++++----------- > ubusd.h | 6 +++--- > ubusd_proto.c | 1 + > 3 files changed, 12 insertions(+), 14 deletions(-) > > diff --git a/ubusd.c b/ubusd.c > index ba1ff07..b7e1f79 100644 > --- a/ubusd.c > +++ b/ubusd.c > @@ -138,11 +138,8 @@ 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); > + struct ubus_msg_buf *qub = ubus_msg_ref(ub); > + list_add_tail(&qub->queue, &cl->tx_queue); > } > > /* takes the msgbuf reference */ > @@ -153,7 +150,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 +169,20 @@ 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; > + return list_first_entry(&cl->tx_queue, struct ubus_msg_buf, queue); > } > > static void ubus_msg_dequeue(struct ubus_client *cl) > { > struct ubus_msg_buf *ub = ubus_msg_head(cl); > - > if (!ub) > return; > > - ubus_msg_free(ub); > + list_del_init(&ub->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(ub); > } > > static void handle_client_disconnect(struct ubus_client *cl) > diff --git a/ubusd.h b/ubusd.h > index 4d87920..375f31f 100644 > --- a/ubusd.h > +++ b/ubusd.h > @@ -23,13 +23,13 @@ > #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 { > uint32_t refcount; /* ~0: uses external data buffer */ > + struct list_head queue; > struct ubus_msghdr hdr; > struct blob_attr *data; > int fd; > @@ -48,8 +48,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; > -- > 2.11.0 > > > _______________________________________________ > Lede-dev mailing list > Lede-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/lede-dev _______________________________________________ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
