MQTT publish causes unnecessary TCP segmentation

[JusbeR]

Describe the bug
For some reason MQTT publishing sends publish packet and its payload in separate packets code

So even if the payload is just 1 byte, then you'll have 2 TCP packets and (usually) 2 TCP ACKs flying in the air to get that one byte delivered. This is a bit of an issue if you are doing this on top of some low bandwidt IoT network.

Maybe there is a good reason to do this, but I can't figure out what that would be.(Just trying out zephyr for the first time)

To Reproduce
Run any MQTT publish code and see the tcpdump.

Expected behavior
One TCP segment to deliver MQTT publish+payload

Impact
Non-optimal network usage & battery consumption

[rlubos]

It's not a bug but conscious use of a STREAM socket features.

Maybe there is a good reason to do this, but I can't figure out what that would be.(Just trying out zephyr for the first time).

In case you wanted to use a single socket call to send the data, you'd need to combine the header and payload in a single, continuous buffer. So you'd either need to make the user prepend the payload buffer with some spare bytes to fill in the header later (and thus increase the complexity for the user) or copy both, the generated header and the payload into a single, preallocated memory area (which might not sound like an issue for a 1-byte payload you describe, but bear in mind that MQTT allows up to nearly 256 MB of payload). We did not want to put any implementation-specific limitation on the payload size.

With this approach you don't need to preallocate or copy anything, you just call the send twice on two separate buffers.

What we really miss here is a Nagle algorithm implementation at TCP layer, which would prevent us from sending small chunks of data into the network. But again, that's a feature request, not a bug.

[JusbeR]

I see and get your point. It is just that the performance effect is quite nasty.

Quick poll on e.g. paho MQTT C stack shows that they indeed use malloc+copy to construct the packet and it does not have this behavior. So is this approach out of the question here? Maybe behind some config flags?

And it is true that API is now brain dead simple and better to keep it that way. But on the other hand, people are already used to

K_DEFINE_MQTT_BUF(my_payloadsize); // Reserves header+payload
write_payload(payload); // writes to payload part

kind of stuff, so maybe that is not impossible either?

Nagle probably wouldn't do anything in this case, as it kicks in only when there already is some data on the line.

[rlubos]

I see, indeed Nagle might not kick in in this particular case.

The simplest approach I can see to tackle this issue is to provide a Kconfig switch (or even a runtime switch within struct mqtt_publish_param) to enable copying of the payload into the TX buffer provided during the initialization. Currently, this buffer is used to construct headers and simple messages (to be honest everything else besides publish).

This should have almost no impact on the implementation or the API, the only caveat would be on the user side, to provide large enough TX buffer to hold both, header and payload. The use case and limitations could be documented, so I don't see an issue here.

I'd like to hear from others about the idea though. If we agree it could work like this, I can provide implementation, it should be pretty straightforward.

[jukkar]

With this approach you don't need to preallocate or copy anything, you just call the send twice on two separate buffers.

What about using sendmsg() instead of two separate send() calls? The sendmsg would normally cause only one TCP segment to be sent.

[rlubos]

What about using sendmsg() instead of two separate send() calls? The sendmsg would normally cause only one TCP segment to be sent.

Yes, that should help in the TCP case. I'm only worried about other transports we have, TLS underneath just calls sendto several times, so there will be no benefit. And the websocket transport does not seem to implement sendmsg at all, so it would require a similar approach as TLS. But we could give it a shot.