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

[Alexandru Ardelean]

On Wed, Jun 7, 2017 at 5:02 PM, Alexandru Ardelean
<ardeleanalex at gmail.com> wrote:
> 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.

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