On Wed, Jun 7, 2017 at 5:02 PM, Alexandru Ardelean <ardeleana...@gmail.com> wrote: > On Wed, Jun 7, 2017 at 4:48 PM, Felix Fietkau <n...@nbd.name> wrote: >> On 2017-06-07 15:44, Felix Fietkau wrote: >>> On 2017-06-07 13:09, Alexandru Ardelean 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). >>>> >>>> Signed-off-by: Alexandru Ardelean <ardeleana...@gmail.com> >>> If I remember correctly, I chose the backlog array because a message can >>> be referenced multiple times (e.g. in the notify/subscribe case). >>> This would break in your linked-list implementation if a message needs >>> to be queued for multiple clients. >> I just re-checked and it seems that multiple references are no longer >> used, so this implementation would probably work. I will take a closer look. > > I also need to take a closer look. > The ubus_msg_ref() incrememts the refcount for non-shared buffers. > So, there may be a loose end I may have missed somewhere. > > To be honest, I'm getting lost a bit within the code sometimes, as it > seems to have been one single file, and gradually split. > And I keep having to fight some wrong assumptions. > > Regarding this dynamic queue. > One idea I was having is to make the array expand [via realloc] up to > a limit [let's say 64k] if needed. > I doubt anyone would use 64k netifd interfaces in a near future. > ~200 sounds almost around the corner of getting there. > Would this be an alternative ? > > Another proposal I would have, is to re-organize the code of ubus > [ubusd mostly] in a series of gradual commits. > The aim would be to reduce shared stuff between files, to make the > code easier to follow. > [ The big thing I am looking at, is the shared `struct blob_buf b;` in > ubusd_proto.c ; that one really makes stuff hard to follow ] > That would maybe make ubusd take up a bit more memory [ don't think > it's too much ], because some more stuff would be alloc-ed per > client/buffer. > I would be glad to do it, if that's fine with you.
So, let's leave this for later. I'll start on the re-organization. I'd prefer we not waste energy on this, since this change is still a bit vague, and not very obvious. Thanks Alex > > Alex > >> >> - Felix _______________________________________________ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev