Cong Wang wrote:

On Wed, May 19, 2021 at 12:06 PM John Fastabend
[off-list ref] wrote:

quoted

Cong Wang wrote:

quoted

On Tue, May 18, 2021 at 12:56 PM John Fastabend
[off-list ref] wrote:

quoted

Cong Wang wrote:

quoted

On Mon, May 17, 2021 at 10:36 PM John Fastabend
[off-list ref] wrote:

quoted

Cong Wang wrote:

quoted

From: Cong Wang <redacted>

sk_psock_verdict_recv() clones the skb and uses the clone
afterward, so udp_read_sock() should free the original skb after
done using it.

The clone only happens if sk_psock_verdict_recv() returns >0.

Sure, in case of error, no one uses the original skb either,
so still need to free it.

But the data is going to be dropped then. I'm questioning if this
is the best we can do or not. Its simplest sure, but could we
do a bit more work and peek those skbs or requeue them? Otherwise
if you cross memory limits for a bit your likely to drop these
unnecessarily.

What are the benefits of not dropping it? When sockmap takes
over sk->sk_data_ready() it should have total control over the skb's
in the receive queue. Otherwise user-space recvmsg() would race
with sockmap when they try to read the first skb at the same time,
therefore potentially user-space could get duplicated data (one via
recvmsg(), one via sockmap). I don't see any benefits but races here.

The benefit of _not_ dropping it is the packet gets to the receiver
side. We've spent a bit of effort to get a packet across the network,
received on the stack, and then we drop it at the last point is not
so friendly.

Well, at least udp_recvmsg() could drop packets too in various
scenarios, for example, a copy error. So, I do not think sockmap
is special.

OK I am at least convinced now that dropping packets is OK and likely
a useful performance/complexity compromise.

But, at this point we wont have any visibility into these drops correct?
Looks like the pattern in UDP stack to handle this is to increment
sk_drops and UDP_MIB_INERRORS. How about we do that here as well?