On Tue, 2024-02-13 at 13:24 +0100, Eric Dumazet wrote:

On Tue, Feb 13, 2024 at 11:49 AM Paolo Abeni wrote:

...

...

@@ -2508,7 +2508,10 @@ static int tcp_recvmsg_locked(struct sock *sk, struct msghdr *msg, size_t len, WRITE_ONCE(*seq, *seq + used); copied += used; len -= used; - + if (flags & MSG_PEEK) + sk_peek_offset_fwd(sk, used); + else + sk_peek_offset_bwd(sk, used);

Yet another cache miss in TCP fast path...

We need to move sk_peek_off in a better location before we accept this patch.

I always thought MSK_PEEK was very inefficient, I am surprised we allow arbitrary loops in recvmsg().

Let me double check I read the above correctly: are you concerned by the 'skb_queue_walk(&sk->sk_receive_queue, skb) {' loop that could touch a lot of skbs/cachelines before reaching the relevant skb? The end goal here is allowing an user-space application to read incrementally/sequentially the received data while leaving them in receive buffer. I don't see a better option than MSG_PEEK, am I missing something? Thanks, Paolo

On Tue, 2024-02-13 at 14:34 +0100, Eric Dumazet wrote:

On Tue, Feb 13, 2024 at 2:02 PM Paolo Abeni wrote:

...

On Tue, 2024-02-13 at 13:24 +0100, Eric Dumazet wrote:

...

On Tue, Feb 13, 2024 at 11:49 AM Paolo Abeni wrote:

...

...

@@ -2508,7 +2508,10 @@ static int tcp_recvmsg_locked(struct sock *sk, struct msghdr *msg, size_t len, WRITE_ONCE(*seq, *seq + used); copied += used; len -= used; - + if (flags & MSG_PEEK) + sk_peek_offset_fwd(sk, used); + else + sk_peek_offset_bwd(sk, used);

Yet another cache miss in TCP fast path...

We need to move sk_peek_off in a better location before we accept this patch.

I always thought MSK_PEEK was very inefficient, I am surprised we allow arbitrary loops in recvmsg().

Let me double check I read the above correctly: are you concerned by the 'skb_queue_walk(&sk->sk_receive_queue, skb) {' loop that could touch a lot of skbs/cachelines before reaching the relevant skb?

The end goal here is allowing an user-space application to read incrementally/sequentially the received data while leaving them in receive buffer.

I don't see a better option than MSG_PEEK, am I missing something?

This sk_peek_offset protocol, needing sk_peek_offset_bwd() in the non MSG_PEEK case is very strange IMO.

Ideally, we should read/write over sk_peek_offset only when MSG_PEEK is used by the caller.

That would only touch non fast paths.

Since the API is mono-threaded anyway, the caller should not rely on the fact that normal recvmsg() call would 'consume' sk_peek_offset.

Storing in sk_peek_seq the tcp next sequence number to be peeked should avoid changes in the non MSG_PEEK cases.  AFAICS that would need a new get_peek_off() sock_op and a bit somewhere (in sk_flags?) to discriminate when sk_peek_seq is actually set. Would that be acceptable? Thanks! Paolo

On Thu, 2024-02-15 at 17:24 -0500, Jon Maloy wrote:

On 2024-02-15 12:46, Eric Dumazet wrote:

...

On Thu, Feb 15, 2024 at 6:41 PM Paolo Abeni wrote:

...

Note: please send text-only email to netdev.

On Thu, 2024-02-15 at 10:11 -0500, Jon Maloy wrote:

...

I wonder if the following could be acceptable:

if (flags & MSG_PEEK) sk_peek_offset_fwd(sk, used); else if (peek_offset > 0) sk_peek_offset_bwd(sk, used);

peek_offset is already present in the data cache, and if it has the value zero it means either that that sk->sk_peek_off is unused (-1) or actually is zero. Either way, no rewind is needed in that case. I agree the above should avoid touching cold cachelines in the fastpath, and looks functionally correct to me.

The last word is up to Eric :)

An actual patch seems needed.

In the current form, local variable peek_offset is 0 when !MSG_PEEK.

So the "else if (peek_offset > 0)" would always be false.

Yes, of course. This wouldn't work unless we read sk->sk_peek_off at the beginning of the function. I will look at the other suggestions.

I *think* that moving sk_peek_off this way: --- diff --git a/include/net/sock.h b/include/net/sock.h index a9d99a9c583f..576a6a6abb03 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -413,7 +413,7 @@ struct sock { unsigned int sk_napi_id; #endif int sk_rcvbuf; - int sk_disconnects; + int sk_peek_off; struct sk_filter __rcu *sk_filter; union { @@ -439,7 +439,7 @@ struct sock { struct rb_root tcp_rtx_queue; }; struct sk_buff_head sk_write_queue; - __s32 sk_peek_off; + int sk_disconnects; int sk_write_pending; __u32 sk_dst_pending_confirm; u32 sk_pacing_status; /* see enum sk_pacing */ --- should avoid problematic accesses, The relevant cachelines layout is as follow: /* --- cacheline 4 boundary (256 bytes) --- */ struct sk_buff * tail; /* 256 8 */ } sk_backlog; /* 240 24 */ int sk_forward_alloc; /* 264 4 */ u32 sk_reserved_mem; /* 268 4 */ unsigned int sk_ll_usec; /* 272 4 */ unsigned int sk_napi_id; /* 276 4 */ int sk_rcvbuf; /* 280 4 */ int sk_disconnects; /* 284 4 */ // will become sk_peek_off struct sk_filter * sk_filter; /* 288 8 */ union { struct socket_wq * sk_wq; /* 296 8 */ struct socket_wq * sk_wq_raw; /* 296 8 */ }; /* 296 8 */ struct xfrm_policy * sk_policy[2]; /* 304 16 */ /* --- cacheline 5 boundary (320 bytes) --- */ // ... /* --- cacheline 6 boundary (384 bytes) --- */ __s32 sk_peek_off; /* 384 4 */ // will become sk_diconnects int sk_write_pending; /* 388 4 */ __u32 sk_dst_pending_confirm; /* 392 4 */ u32 sk_pacing_status; /* 396 4 */ long int sk_sndtimeo; /* 400 8 */ struct timer_list sk_timer; /* 408 40 */ /* XXX last struct has 4 bytes of padding */ /* --- cacheline 7 boundary (448 bytes) --- */ sk_peek_off will be in the same cachline of sk_forward_alloc / sk_reserved_mem / backlog tail, that are already touched by the tcp_recvmsg_locked() main loop. WDYT? thanks! Paolo

On 2024-02-16 04:21, Eric Dumazet wrote:

On Fri, Feb 16, 2024 at 10:14 AM Paolo Abeni wrote:

...

On Thu, 2024-02-15 at 17:24 -0500, Jon Maloy wrote:

...

On 2024-02-15 12:46, Eric Dumazet wrote:

...

On Thu, Feb 15, 2024 at 6:41 PM Paolo Abeni wrote:

...

Note: please send text-only email to netdev.

On Thu, 2024-02-15 at 10:11 -0500, Jon Maloy wrote:

...

I wonder if the following could be acceptable:

if (flags & MSG_PEEK) sk_peek_offset_fwd(sk, used); else if (peek_offset > 0) sk_peek_offset_bwd(sk, used);

peek_offset is already present in the data cache, and if it has the value zero it means either that that sk->sk_peek_off is unused (-1) or actually is zero. Either way, no rewind is needed in that case. I agree the above should avoid touching cold cachelines in the fastpath, and looks functionally correct to me.

The last word is up to Eric :)

An actual patch seems needed.

In the current form, local variable peek_offset is 0 when !MSG_PEEK.

So the "else if (peek_offset > 0)" would always be false.

Yes, of course. This wouldn't work unless we read sk->sk_peek_off at the beginning of the function. I will look at the other suggestions. I *think* that moving sk_peek_off this way:

--- diff --git a/include/net/sock.h b/include/net/sock.h index a9d99a9c583f..576a6a6abb03 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -413,7 +413,7 @@ struct sock { unsigned int sk_napi_id; #endif int sk_rcvbuf; - int sk_disconnects; + int sk_peek_off;

struct sk_filter __rcu *sk_filter; union { @@ -439,7 +439,7 @@ struct sock { struct rb_root tcp_rtx_queue; }; struct sk_buff_head sk_write_queue; - __s32 sk_peek_off; + int sk_disconnects; int sk_write_pending; __u32 sk_dst_pending_confirm; u32 sk_pacing_status; /* see enum sk_pacing */ ---

should avoid problematic accesses,

The relevant cachelines layout is as follow:

/* --- cacheline 4 boundary (256 bytes) --- */ struct sk_buff * tail; /* 256 8 */ } sk_backlog; /* 240 24 */ int sk_forward_alloc; /* 264 4 */ u32 sk_reserved_mem; /* 268 4 */ unsigned int sk_ll_usec; /* 272 4 */ unsigned int sk_napi_id; /* 276 4 */ int sk_rcvbuf; /* 280 4 */ int sk_disconnects; /* 284 4 */ // will become sk_peek_off struct sk_filter * sk_filter; /* 288 8 */ union { struct socket_wq * sk_wq; /* 296 8 */ struct socket_wq * sk_wq_raw; /* 296 8 */ }; /* 296 8 */ struct xfrm_policy * sk_policy[2]; /* 304 16 */ /* --- cacheline 5 boundary (320 bytes) --- */

// ...

/* --- cacheline 6 boundary (384 bytes) --- */ __s32 sk_peek_off; /* 384 4 */ // will become sk_diconnects int sk_write_pending; /* 388 4 */ __u32 sk_dst_pending_confirm; /* 392 4 */ u32 sk_pacing_status; /* 396 4 */ long int sk_sndtimeo; /* 400 8 */ struct timer_list sk_timer; /* 408 40 */

/* XXX last struct has 4 bytes of padding */

/* --- cacheline 7 boundary (448 bytes) --- */

sk_peek_off will be in the same cachline of sk_forward_alloc / sk_reserved_mem / backlog tail, that are already touched by the tcp_recvmsg_locked() main loop.

WDYT? I was about to send a similar change, also moving sk_rcvtimeo, and adding __cacheline_group_begin()/__cacheline_group_end annotations.

I can finish this today.

There is also the following alternative: if (flags & MSG_PEEK)        sk_peek_offset_fwd(sk, used); else if (flags & MSG_TRUNC)        sk_peek_offset_bwd(sk, used); This is the way we use it, and probably the typical usage. It would force a user to drain the receive queue with MSG_TRUNC whenever he is using MSG_PEEK_OFF, but I don't really see that as a limitation. Anyway, if Paolo's suggestion solves the problem this shouldn't be necessary. ///jon

On Fri, Feb 16, 2024 at 05:13:34AM -0500, Jon Maloy wrote:

On 2024-02-16 04:21, Eric Dumazet wrote:

...

On Fri, Feb 16, 2024 at 10:14 AM Paolo Abeni wrote:

...

On Thu, 2024-02-15 at 17:24 -0500, Jon Maloy wrote:

...

On 2024-02-15 12:46, Eric Dumazet wrote:

...

On Thu, Feb 15, 2024 at 6:41 PM Paolo Abeni wrote:

...

Note: please send text-only email to netdev.

On Thu, 2024-02-15 at 10:11 -0500, Jon Maloy wrote: > I wonder if the following could be acceptable: > > if (flags & MSG_PEEK) > sk_peek_offset_fwd(sk, used); > else if (peek_offset > 0) > sk_peek_offset_bwd(sk, used); > > peek_offset is already present in the data cache, and if it has the value > zero it means either that that sk->sk_peek_off is unused (-1) or actually is zero. > Either way, no rewind is needed in that case. I agree the above should avoid touching cold cachelines in the fastpath, and looks functionally correct to me.

The last word is up to Eric :)

An actual patch seems needed.

In the current form, local variable peek_offset is 0 when !MSG_PEEK.

So the "else if (peek_offset > 0)" would always be false.

Yes, of course. This wouldn't work unless we read sk->sk_peek_off at the beginning of the function. I will look at the other suggestions. I *think* that moving sk_peek_off this way:

--- diff --git a/include/net/sock.h b/include/net/sock.h index a9d99a9c583f..576a6a6abb03 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -413,7 +413,7 @@ struct sock { unsigned int sk_napi_id; #endif int sk_rcvbuf; - int sk_disconnects; + int sk_peek_off;

struct sk_filter __rcu *sk_filter; union { @@ -439,7 +439,7 @@ struct sock { struct rb_root tcp_rtx_queue; }; struct sk_buff_head sk_write_queue; - __s32 sk_peek_off; + int sk_disconnects; int sk_write_pending; __u32 sk_dst_pending_confirm; u32 sk_pacing_status; /* see enum sk_pacing */ ---

should avoid problematic accesses,

The relevant cachelines layout is as follow:

/* --- cacheline 4 boundary (256 bytes) --- */ struct sk_buff * tail; /* 256 8 */ } sk_backlog; /* 240 24 */ int sk_forward_alloc; /* 264 4 */ u32 sk_reserved_mem; /* 268 4 */ unsigned int sk_ll_usec; /* 272 4 */ unsigned int sk_napi_id; /* 276 4 */ int sk_rcvbuf; /* 280 4 */ int sk_disconnects; /* 284 4 */ // will become sk_peek_off struct sk_filter * sk_filter; /* 288 8 */ union { struct socket_wq * sk_wq; /* 296 8 */ struct socket_wq * sk_wq_raw; /* 296 8 */ }; /* 296 8 */ struct xfrm_policy * sk_policy[2]; /* 304 16 */ /* --- cacheline 5 boundary (320 bytes) --- */

// ...

/* --- cacheline 6 boundary (384 bytes) --- */ __s32 sk_peek_off; /* 384 4 */ // will become sk_diconnects int sk_write_pending; /* 388 4 */ __u32 sk_dst_pending_confirm; /* 392 4 */ u32 sk_pacing_status; /* 396 4 */ long int sk_sndtimeo; /* 400 8 */ struct timer_list sk_timer; /* 408 40 */

/* XXX last struct has 4 bytes of padding */

/* --- cacheline 7 boundary (448 bytes) --- */

sk_peek_off will be in the same cachline of sk_forward_alloc / sk_reserved_mem / backlog tail, that are already touched by the tcp_recvmsg_locked() main loop.

WDYT? I was about to send a similar change, also moving sk_rcvtimeo, and adding __cacheline_group_begin()/__cacheline_group_end annotations.

I can finish this today.

There is also the following alternative:

if (flags & MSG_PEEK)        sk_peek_offset_fwd(sk, used); else if (flags & MSG_TRUNC)        sk_peek_offset_bwd(sk, used);

This is the way we use it, and probably the typical usage. It would force a user to drain the receive queue with MSG_TRUNC whenever he is using MSG_PEEK_OFF, but I don't really see that as a limitation.

I really don't like this, although it would certainly do what we need for passt/pasta. SO_PEEK_OFF has established semantics for Unix sockets, which includes regular recv() adjusting the offset. Having it behave subtlety differently for TCP seems like a very bad idea.

Anyway, if Paolo's suggestion solves the problem this shouldn't be necessary.

///jon

-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

On Wed, Feb 14, 2024 at 10:34:50AM +1100, David Gibson wrote:

On Tue, Feb 13, 2024 at 04:49:01PM +0100, Eric Dumazet wrote:

...

On Tue, Feb 13, 2024 at 4:28 PM Paolo Abeni wrote:

...

On Tue, 2024-02-13 at 14:34 +0100, Eric Dumazet wrote:

...

On Tue, Feb 13, 2024 at 2:02 PM Paolo Abeni wrote:

...

On Tue, 2024-02-13 at 13:24 +0100, Eric Dumazet wrote:

...

On Tue, Feb 13, 2024 at 11:49 AM Paolo Abeni wrote:

> > @@ -2508,7 +2508,10 @@ static int tcp_recvmsg_locked(struct sock *sk, struct msghdr *msg, size_t len, > > WRITE_ONCE(*seq, *seq + used); > > copied += used; > > len -= used; > > - > > + if (flags & MSG_PEEK) > > + sk_peek_offset_fwd(sk, used); > > + else > > + sk_peek_offset_bwd(sk, used);

Yet another cache miss in TCP fast path...

We need to move sk_peek_off in a better location before we accept this patch.

I always thought MSK_PEEK was very inefficient, I am surprised we allow arbitrary loops in recvmsg().

Let me double check I read the above correctly: are you concerned by the 'skb_queue_walk(&sk->sk_receive_queue, skb) {' loop that could touch a lot of skbs/cachelines before reaching the relevant skb?

The end goal here is allowing an user-space application to read incrementally/sequentially the received data while leaving them in receive buffer.

I don't see a better option than MSG_PEEK, am I missing something?

This sk_peek_offset protocol, needing sk_peek_offset_bwd() in the non MSG_PEEK case is very strange IMO.

Ideally, we should read/write over sk_peek_offset only when MSG_PEEK is used by the caller.

That would only touch non fast paths.

Since the API is mono-threaded anyway, the caller should not rely on the fact that normal recvmsg() call would 'consume' sk_peek_offset.

Storing in sk_peek_seq the tcp next sequence number to be peeked should avoid changes in the non MSG_PEEK cases.

AFAICS that would need a new get_peek_off() sock_op and a bit somewhere (in sk_flags?) to discriminate when sk_peek_seq is actually set. Would that be acceptable?

We could have a parallel SO_PEEK_OFFSET option, reusing the same socket field.

The new semantic would be : Supported by TCP (so far), and tcp recvmsg() only reads/writes this field when MSG_PEEK is used. Applications would have to clear the values themselves.

Those semantics would likely defeat the purpose of using SO_PEEK_OFF for our use case, since we'd need an additional setsockopt() for every non-PEEK recv() (which are all MSG_TRUNC in our case).

Btw, Eric, If you're concerned about the extra access added to the "regular" TCP path, would you be happier with the original approach Jon proposed: that allowed a user to essentially supply an offset to an individial MSG_PEEK recvmsg() by inserting a dummy entry as msg_iov[0] with a NULL pointer and length to skip. It did the job for us, although I admit it's a little ugly, which I presume is why Paolo suggested we investigate SO_PEEK_OFF instead. I think the SO_PEEK_OFF approach is more elegant, but maybe the performance impact outweighs that. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson