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

[Alexandru Ardelean]

On Wed, Jun 7, 2017 at 4:48 PM, Felix Fietkau <nbd at 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 <ardeleanalex at 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.

Alex

>
> - Felix