Re: [PATCH v2 1/2] udp: UDP socket send queue repair

On Fri, Aug 13, 2021 at 4:52 AM Bui Quang Minh [off-list ref] wrote:

On 8/12/2021 10:51 PM, Eric Dumazet wrote:

quoted

On 8/12/21 3:46 PM, Bui Quang Minh wrote:

quoted

On 8/11/2021 11:14 PM, Eric Dumazet wrote:

quoted

On 8/11/21 5:45 PM, Bui Quang Minh wrote:

quoted

In this patch, I implement UDP_REPAIR sockoption and a new path in
udp_recvmsg for dumping the corked packet in UDP socket's send queue.

A userspace program can use recvmsg syscall to get the packet's data and
the msg_name information of the packet. Currently, other related
information in inet_cork that are set in cmsg are not dumped.

While working on this, I was aware of Lese Doru Calin's patch and got some
ideas from it.

What is the use case for this feature, adding a test in UDP fast path ?

This feature is used to help CRIU to dump CORKed UDP packet in send queue. I'm sorry for being not aware of the performance perspective here.

UDP is not reliable.

I find a bit strange we add so many lines of code
for a feature trying very hard to to drop _one_ packet.

I think a much better changelog would be welcomed.

The reason we want to dump the packet in send queue is to make to state of the
application consistent. The scenario is that when an application sends UDP
packets via UDP_CORK socket or with MSG_MORE, CRIU comes and checkpoints the
application. If we drop the data in send queue, when application restores, it
sends some more data then turns off the cork and actually sends a packet. The
receiving side may get that packet but it's unusual that the first part of that
packet is missing because we drop it. So we try to solve this problem with some
help from the Linux kernel.

Instead of checkpointing the state, how about making the kernel drop
the next packet.

For instance by setting up->pending to something else than AF_UNSPEC,
AF_INET, AF_INET6 from a new setsockopt and testing for this case in
the udp_sendmsg up->pending slowpath.

udp_sendmsg already calls udp_v6_flush_pending_frames on error when
appending to a pending packet, so returning an error on the next call
after restore and have that imply a flush is acceptable. I would
introduce a new error code.

The state can perhaps be inferred in other ways, e.g., from
up->pending && !up->len or up->pending && !skb_peek_tail(queue). But
an explicit up->pending mode will be easier to grasp.