* [net-next v2 0/2] tcp: add CMSG+rx timestamps to rx. zerocopy
@ 2021-01-21 0:41 Arjun Roy
2021-01-21 0:41 ` [net-next v2 1/2] tcp: Remove CMSG magic numbers for tcp_recvmsg() Arjun Roy
2021-01-21 0:41 ` [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy Arjun Roy
0 siblings, 2 replies; 12+ messages in thread
From: Arjun Roy @ 2021-01-21 0:41 UTC (permalink / raw)
To: davem, netdev; +Cc: arjunroy, edumazet, soheil, kuba
From: Arjun Roy <arjunroy@google.com>
Provide CMSG and receive timestamp support to TCP
receive zerocopy. Patch 1 refactors CMSG pending state for
tcp_recvmsg() to avoid the use of magic numbers; patch 2 implements
receive timestamp via CMSG support for receive zerocopy, and uses the
constants added in patch 1.
v2: Fixes various stylistic comments and introduces a helper method
to reduce indentation.
Arjun Roy (2):
tcp: Remove CMSG magic numbers for tcp_recvmsg().
tcp: Add receive timestamp support for receive zerocopy.
include/uapi/linux/tcp.h | 4 ++
net/ipv4/tcp.c | 130 ++++++++++++++++++++++++++++-----------
2 files changed, 98 insertions(+), 36 deletions(-)
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply [flat|nested] 12+ messages in thread
* [net-next v2 1/2] tcp: Remove CMSG magic numbers for tcp_recvmsg().
2021-01-21 0:41 [net-next v2 0/2] tcp: add CMSG+rx timestamps to rx. zerocopy Arjun Roy
@ 2021-01-21 0:41 ` Arjun Roy
2021-01-21 0:41 ` [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy Arjun Roy
1 sibling, 0 replies; 12+ messages in thread
From: Arjun Roy @ 2021-01-21 0:41 UTC (permalink / raw)
To: davem, netdev; +Cc: arjunroy, edumazet, soheil, kuba
From: Arjun Roy <arjunroy@google.com>
At present, tcp_recvmsg() uses flags to track if any CMSGs are pending
and what those CMSGs are. These flags are currently magic numbers,
used only within tcp_recvmsg().
To prepare for receive timestamp support in tcp receive zerocopy,
gently refactor these magic numbers into enums.
Signed-off-by: Arjun Roy <arjunroy@google.com>
---
net/ipv4/tcp.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 856ae516ac18..28ca6a024f63 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -280,6 +280,12 @@
#include <asm/ioctls.h>
#include <net/busy_poll.h>
+/* Track pending CMSGs. */
+enum {
+ TCP_CMSG_INQ = 1,
+ TCP_CMSG_TS = 2
+};
+
struct percpu_counter tcp_orphan_count;
EXPORT_SYMBOL_GPL(tcp_orphan_count);
@@ -2272,7 +2278,7 @@ static int tcp_recvmsg_locked(struct sock *sk, struct msghdr *msg, size_t len,
goto out;
if (tp->recvmsg_inq)
- *cmsg_flags = 1;
+ *cmsg_flags = TCP_CMSG_INQ;
timeo = sock_rcvtimeo(sk, nonblock);
/* Urgent data needs to be handled specially. */
@@ -2453,7 +2459,7 @@ static int tcp_recvmsg_locked(struct sock *sk, struct msghdr *msg, size_t len,
if (TCP_SKB_CB(skb)->has_rxtstamp) {
tcp_update_recv_tstamps(skb, tss);
- *cmsg_flags |= 2;
+ *cmsg_flags |= TCP_CMSG_TS;
}
if (used + offset < skb->len)
@@ -2513,9 +2519,9 @@ int tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int nonblock,
release_sock(sk);
if (cmsg_flags && ret >= 0) {
- if (cmsg_flags & 2)
+ if (cmsg_flags & TCP_CMSG_TS)
tcp_recv_timestamp(msg, sk, &tss);
- if (cmsg_flags & 1) {
+ if (cmsg_flags & TCP_CMSG_INQ) {
inq = tcp_inq_hint(sk);
put_cmsg(msg, SOL_TCP, TCP_CM_INQ, sizeof(inq), &inq);
}
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy.
2021-01-21 0:41 [net-next v2 0/2] tcp: add CMSG+rx timestamps to rx. zerocopy Arjun Roy
2021-01-21 0:41 ` [net-next v2 1/2] tcp: Remove CMSG magic numbers for tcp_recvmsg() Arjun Roy
@ 2021-01-21 0:41 ` Arjun Roy
2021-01-23 4:07 ` Jakub Kicinski
1 sibling, 1 reply; 12+ messages in thread
From: Arjun Roy @ 2021-01-21 0:41 UTC (permalink / raw)
To: davem, netdev; +Cc: arjunroy, edumazet, soheil, kuba
From: Arjun Roy <arjunroy@google.com>
tcp_recvmsg() uses the CMSG mechanism to receive control information
like packet receive timestamps. This patch adds CMSG fields to
struct tcp_zerocopy_receive, and provides receive timestamps
if available to the user.
Signed-off-by: Arjun Roy <arjunroy@google.com>
---
include/uapi/linux/tcp.h | 4 ++
net/ipv4/tcp.c | 116 ++++++++++++++++++++++++++++-----------
2 files changed, 88 insertions(+), 32 deletions(-)
diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
index 768e93bd5b51..b216270105af 100644
--- a/include/uapi/linux/tcp.h
+++ b/include/uapi/linux/tcp.h
@@ -353,5 +353,9 @@ struct tcp_zerocopy_receive {
__u64 copybuf_address; /* in: copybuf address (small reads) */
__s32 copybuf_len; /* in/out: copybuf bytes avail/used or error */
__u32 flags; /* in: flags */
+ __u64 msg_control; /* ancillary data */
+ __u64 msg_controllen;
+ __u32 msg_flags;
+ /* __u32 hole; Next we must add >1 u32 otherwise length checks fail. */
};
#endif /* _UAPI_LINUX_TCP_H */
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 28ca6a024f63..0e6f9b8d9f43 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1745,6 +1745,20 @@ int tcp_set_rcvlowat(struct sock *sk, int val)
}
EXPORT_SYMBOL(tcp_set_rcvlowat);
+static void tcp_update_recv_tstamps(struct sk_buff *skb,
+ struct scm_timestamping_internal *tss)
+{
+ if (skb->tstamp)
+ tss->ts[0] = ktime_to_timespec64(skb->tstamp);
+ else
+ tss->ts[0] = (struct timespec64) {0};
+
+ if (skb_hwtstamps(skb)->hwtstamp)
+ tss->ts[2] = ktime_to_timespec64(skb_hwtstamps(skb)->hwtstamp);
+ else
+ tss->ts[2] = (struct timespec64) {0};
+}
+
#ifdef CONFIG_MMU
static const struct vm_operations_struct tcp_vm_ops = {
};
@@ -1848,13 +1862,13 @@ static int tcp_recvmsg_locked(struct sock *sk, struct msghdr *msg, size_t len,
struct scm_timestamping_internal *tss,
int *cmsg_flags);
static int receive_fallback_to_copy(struct sock *sk,
- struct tcp_zerocopy_receive *zc, int inq)
+ struct tcp_zerocopy_receive *zc, int inq,
+ struct scm_timestamping_internal *tss)
{
unsigned long copy_address = (unsigned long)zc->copybuf_address;
- struct scm_timestamping_internal tss_unused;
- int err, cmsg_flags_unused;
struct msghdr msg = {};
struct iovec iov;
+ int err;
zc->length = 0;
zc->recv_skip_hint = 0;
@@ -1868,7 +1882,7 @@ static int receive_fallback_to_copy(struct sock *sk,
return err;
err = tcp_recvmsg_locked(sk, &msg, inq, /*nonblock=*/1, /*flags=*/0,
- &tss_unused, &cmsg_flags_unused);
+ tss, &zc->msg_flags);
if (err < 0)
return err;
@@ -1909,21 +1923,27 @@ static int tcp_copy_straggler_data(struct tcp_zerocopy_receive *zc,
return (__s32)copylen;
}
-static int tcp_zerocopy_handle_leftover_data(struct tcp_zerocopy_receive *zc,
- struct sock *sk,
- struct sk_buff *skb,
- u32 *seq,
- s32 copybuf_len)
+static int tcp_zc_handle_leftover(struct tcp_zerocopy_receive *zc,
+ struct sock *sk,
+ struct sk_buff *skb,
+ u32 *seq,
+ s32 copybuf_len,
+ struct scm_timestamping_internal *tss)
{
u32 offset, copylen = min_t(u32, copybuf_len, zc->recv_skip_hint);
if (!copylen)
return 0;
/* skb is null if inq < PAGE_SIZE. */
- if (skb)
+ if (skb) {
offset = *seq - TCP_SKB_CB(skb)->seq;
- else
+ } else {
skb = tcp_recv_skb(sk, *seq, &offset);
+ if (TCP_SKB_CB(skb)->has_rxtstamp) {
+ tcp_update_recv_tstamps(skb, tss);
+ zc->msg_flags |= TCP_CMSG_TS;
+ }
+ }
zc->copybuf_len = tcp_copy_straggler_data(zc, skb, copylen, &offset,
seq);
@@ -2010,9 +2030,37 @@ static int tcp_zerocopy_vm_insert_batch(struct vm_area_struct *vma,
err);
}
+static void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
+ struct scm_timestamping_internal *tss);
+static void tcp_zc_finalize_rx_tstamp(struct sock *sk,
+ struct tcp_zerocopy_receive *zc,
+ struct scm_timestamping_internal *tss)
+{
+ unsigned long msg_control_addr;
+ struct msghdr cmsg_dummy;
+
+ msg_control_addr = (unsigned long)zc->msg_control;
+ cmsg_dummy.msg_control = (void *)msg_control_addr;
+ cmsg_dummy.msg_controllen =
+ (__kernel_size_t)zc->msg_controllen;
+ cmsg_dummy.msg_flags = in_compat_syscall()
+ ? MSG_CMSG_COMPAT : 0;
+ zc->msg_flags = 0;
+ if (zc->msg_control == msg_control_addr &&
+ zc->msg_controllen == cmsg_dummy.msg_controllen) {
+ tcp_recv_timestamp(&cmsg_dummy, sk, tss);
+ zc->msg_control = (__u64)
+ ((uintptr_t)cmsg_dummy.msg_control);
+ zc->msg_controllen =
+ (__u64)cmsg_dummy.msg_controllen;
+ zc->msg_flags = (__u32)cmsg_dummy.msg_flags;
+ }
+}
+
#define TCP_ZEROCOPY_PAGE_BATCH_SIZE 32
static int tcp_zerocopy_receive(struct sock *sk,
- struct tcp_zerocopy_receive *zc)
+ struct tcp_zerocopy_receive *zc,
+ struct scm_timestamping_internal *tss)
{
u32 length = 0, offset, vma_len, avail_len, copylen = 0;
unsigned long address = (unsigned long)zc->address;
@@ -2029,6 +2077,7 @@ static int tcp_zerocopy_receive(struct sock *sk,
int ret;
zc->copybuf_len = 0;
+ zc->msg_flags = 0;
if (address & (PAGE_SIZE - 1) || address != zc->address)
return -EINVAL;
@@ -2039,7 +2088,7 @@ static int tcp_zerocopy_receive(struct sock *sk,
sock_rps_record_flow(sk);
if (inq && inq <= copybuf_len)
- return receive_fallback_to_copy(sk, zc, inq);
+ return receive_fallback_to_copy(sk, zc, inq, tss);
if (inq < PAGE_SIZE) {
zc->length = 0;
@@ -2084,6 +2133,11 @@ static int tcp_zerocopy_receive(struct sock *sk,
} else {
skb = tcp_recv_skb(sk, seq, &offset);
}
+
+ if (TCP_SKB_CB(skb)->has_rxtstamp) {
+ tcp_update_recv_tstamps(skb, tss);
+ zc->msg_flags |= TCP_CMSG_TS;
+ }
zc->recv_skip_hint = skb->len - offset;
frags = skb_advance_to_frag(skb, offset, &offset_frag);
if (!frags || offset_frag)
@@ -2126,8 +2180,7 @@ static int tcp_zerocopy_receive(struct sock *sk,
mmap_read_unlock(current->mm);
/* Try to copy straggler data. */
if (!ret)
- copylen = tcp_zerocopy_handle_leftover_data(zc, sk, skb, &seq,
- copybuf_len);
+ copylen = tcp_zc_handle_leftover(zc, sk, skb, &seq, copybuf_len, tss);
if (length + copylen) {
WRITE_ONCE(tp->copied_seq, seq);
@@ -2148,20 +2201,6 @@ static int tcp_zerocopy_receive(struct sock *sk,
}
#endif
-static void tcp_update_recv_tstamps(struct sk_buff *skb,
- struct scm_timestamping_internal *tss)
-{
- if (skb->tstamp)
- tss->ts[0] = ktime_to_timespec64(skb->tstamp);
- else
- tss->ts[0] = (struct timespec64) {0};
-
- if (skb_hwtstamps(skb)->hwtstamp)
- tss->ts[2] = ktime_to_timespec64(skb_hwtstamps(skb)->hwtstamp);
- else
- tss->ts[2] = (struct timespec64) {0};
-}
-
/* Similar to __sock_recv_timestamp, but does not require an skb */
static void tcp_recv_timestamp(struct msghdr *msg, const struct sock *sk,
struct scm_timestamping_internal *tss)
@@ -4089,6 +4128,7 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
}
#ifdef CONFIG_MMU
case TCP_ZEROCOPY_RECEIVE: {
+ struct scm_timestamping_internal tss;
struct tcp_zerocopy_receive zc = {};
int err;
@@ -4104,11 +4144,18 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
if (copy_from_user(&zc, optval, len))
return -EFAULT;
lock_sock(sk);
- err = tcp_zerocopy_receive(sk, &zc);
+ err = tcp_zerocopy_receive(sk, &zc, &tss);
release_sock(sk);
- if (len >= offsetofend(struct tcp_zerocopy_receive, err))
- goto zerocopy_rcv_sk_err;
+ if (len >= offsetofend(struct tcp_zerocopy_receive, msg_flags))
+ goto zerocopy_rcv_cmsg;
switch (len) {
+ case offsetofend(struct tcp_zerocopy_receive, msg_flags):
+ goto zerocopy_rcv_cmsg;
+ case offsetofend(struct tcp_zerocopy_receive, msg_controllen):
+ case offsetofend(struct tcp_zerocopy_receive, msg_control):
+ case offsetofend(struct tcp_zerocopy_receive, flags):
+ case offsetofend(struct tcp_zerocopy_receive, copybuf_len):
+ case offsetofend(struct tcp_zerocopy_receive, copybuf_address):
case offsetofend(struct tcp_zerocopy_receive, err):
goto zerocopy_rcv_sk_err;
case offsetofend(struct tcp_zerocopy_receive, inq):
@@ -4117,6 +4164,11 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
default:
goto zerocopy_rcv_out;
}
+zerocopy_rcv_cmsg:
+ if (zc.msg_flags & TCP_CMSG_TS)
+ tcp_zc_finalize_rx_tstamp(sk, &zc, &tss);
+ else
+ zc.msg_flags = 0;
zerocopy_rcv_sk_err:
if (!err)
zc.err = sock_error(sk);
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy.
2021-01-21 0:41 ` [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy Arjun Roy
@ 2021-01-23 4:07 ` Jakub Kicinski
2021-01-23 5:55 ` David Ahern
0 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2021-01-23 4:07 UTC (permalink / raw)
To: Arjun Roy; +Cc: davem, netdev, arjunroy, edumazet, soheil
On Wed, 20 Jan 2021 16:41:48 -0800 Arjun Roy wrote:
> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> index 768e93bd5b51..b216270105af 100644
> --- a/include/uapi/linux/tcp.h
> +++ b/include/uapi/linux/tcp.h
> @@ -353,5 +353,9 @@ struct tcp_zerocopy_receive {
> __u64 copybuf_address; /* in: copybuf address (small reads) */
> __s32 copybuf_len; /* in/out: copybuf bytes avail/used or error */
> __u32 flags; /* in: flags */
> + __u64 msg_control; /* ancillary data */
> + __u64 msg_controllen;
> + __u32 msg_flags;
> + /* __u32 hole; Next we must add >1 u32 otherwise length checks fail. */
Well, let's hope nobody steps on this landmine.. :)
Applied, thanks!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy.
2021-01-23 4:07 ` Jakub Kicinski
@ 2021-01-23 5:55 ` David Ahern
2021-01-25 6:15 ` Leon Romanovsky
0 siblings, 1 reply; 12+ messages in thread
From: David Ahern @ 2021-01-23 5:55 UTC (permalink / raw)
To: Jakub Kicinski, Arjun Roy; +Cc: davem, netdev, arjunroy, edumazet, soheil
On 1/22/21 9:07 PM, Jakub Kicinski wrote:
> On Wed, 20 Jan 2021 16:41:48 -0800 Arjun Roy wrote:
>> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
>> index 768e93bd5b51..b216270105af 100644
>> --- a/include/uapi/linux/tcp.h
>> +++ b/include/uapi/linux/tcp.h
>> @@ -353,5 +353,9 @@ struct tcp_zerocopy_receive {
>> __u64 copybuf_address; /* in: copybuf address (small reads) */
>> __s32 copybuf_len; /* in/out: copybuf bytes avail/used or error */
>> __u32 flags; /* in: flags */
>> + __u64 msg_control; /* ancillary data */
>> + __u64 msg_controllen;
>> + __u32 msg_flags;
>> + /* __u32 hole; Next we must add >1 u32 otherwise length checks fail. */
>
> Well, let's hope nobody steps on this landmine.. :)
>
Past suggestions were made to use anonymous declarations - e.g., __u32
:32; - as a way of reserving the space for future use. That or declare
'__u32 resvd', check that it must be 0 and makes it available for later
(either directly or with a union).
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy.
2021-01-23 5:55 ` David Ahern
@ 2021-01-25 6:15 ` Leon Romanovsky
2021-02-02 2:06 ` David Ahern
0 siblings, 1 reply; 12+ messages in thread
From: Leon Romanovsky @ 2021-01-25 6:15 UTC (permalink / raw)
To: David Ahern
Cc: Jakub Kicinski, Arjun Roy, davem, netdev, arjunroy, edumazet, soheil
On Fri, Jan 22, 2021 at 10:55:45PM -0700, David Ahern wrote:
> On 1/22/21 9:07 PM, Jakub Kicinski wrote:
> > On Wed, 20 Jan 2021 16:41:48 -0800 Arjun Roy wrote:
> >> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> >> index 768e93bd5b51..b216270105af 100644
> >> --- a/include/uapi/linux/tcp.h
> >> +++ b/include/uapi/linux/tcp.h
> >> @@ -353,5 +353,9 @@ struct tcp_zerocopy_receive {
> >> __u64 copybuf_address; /* in: copybuf address (small reads) */
> >> __s32 copybuf_len; /* in/out: copybuf bytes avail/used or error */
> >> __u32 flags; /* in: flags */
> >> + __u64 msg_control; /* ancillary data */
> >> + __u64 msg_controllen;
> >> + __u32 msg_flags;
> >> + /* __u32 hole; Next we must add >1 u32 otherwise length checks fail. */
> >
> > Well, let's hope nobody steps on this landmine.. :)
> >
>
> Past suggestions were made to use anonymous declarations - e.g., __u32
> :32; - as a way of reserving the space for future use. That or declare
> '__u32 resvd', check that it must be 0 and makes it available for later
> (either directly or with a union).
This is the schema (reserved field without union) used by the RDMA UAPIs from
the beginning (>20 years already) and it works like a charm.
Highly recommend :).
Thanks
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy.
2021-01-25 6:15 ` Leon Romanovsky
@ 2021-02-02 2:06 ` David Ahern
2021-02-02 2:20 ` Arjun Roy
0 siblings, 1 reply; 12+ messages in thread
From: David Ahern @ 2021-02-02 2:06 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Jakub Kicinski, Arjun Roy, davem, netdev, arjunroy, edumazet, soheil
On 1/24/21 11:15 PM, Leon Romanovsky wrote:
> On Fri, Jan 22, 2021 at 10:55:45PM -0700, David Ahern wrote:
>> On 1/22/21 9:07 PM, Jakub Kicinski wrote:
>>> On Wed, 20 Jan 2021 16:41:48 -0800 Arjun Roy wrote:
>>>> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
>>>> index 768e93bd5b51..b216270105af 100644
>>>> --- a/include/uapi/linux/tcp.h
>>>> +++ b/include/uapi/linux/tcp.h
>>>> @@ -353,5 +353,9 @@ struct tcp_zerocopy_receive {
>>>> __u64 copybuf_address; /* in: copybuf address (small reads) */
>>>> __s32 copybuf_len; /* in/out: copybuf bytes avail/used or error */
>>>> __u32 flags; /* in: flags */
>>>> + __u64 msg_control; /* ancillary data */
>>>> + __u64 msg_controllen;
>>>> + __u32 msg_flags;
>>>> + /* __u32 hole; Next we must add >1 u32 otherwise length checks fail. */
>>>
>>> Well, let's hope nobody steps on this landmine.. :)
>>>
>>
>> Past suggestions were made to use anonymous declarations - e.g., __u32
>> :32; - as a way of reserving the space for future use. That or declare
>> '__u32 resvd', check that it must be 0 and makes it available for later
>> (either directly or with a union).
>
> This is the schema (reserved field without union) used by the RDMA UAPIs from
> the beginning (>20 years already) and it works like a charm.
>
> Highly recommend :).
>
agreed.
Arjun: would you mind following up with a patch to make this hole
explicit and usable for the next extension? Thanks,
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy.
2021-02-02 2:06 ` David Ahern
@ 2021-02-02 2:20 ` Arjun Roy
2021-02-02 6:52 ` Leon Romanovsky
0 siblings, 1 reply; 12+ messages in thread
From: Arjun Roy @ 2021-02-02 2:20 UTC (permalink / raw)
To: David Ahern
Cc: Leon Romanovsky, Jakub Kicinski, Arjun Roy, David Miller, netdev,
Eric Dumazet, Soheil Hassas Yeganeh
On Mon, Feb 1, 2021 at 6:06 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 1/24/21 11:15 PM, Leon Romanovsky wrote:
> > On Fri, Jan 22, 2021 at 10:55:45PM -0700, David Ahern wrote:
> >> On 1/22/21 9:07 PM, Jakub Kicinski wrote:
> >>> On Wed, 20 Jan 2021 16:41:48 -0800 Arjun Roy wrote:
> >>>> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> >>>> index 768e93bd5b51..b216270105af 100644
> >>>> --- a/include/uapi/linux/tcp.h
> >>>> +++ b/include/uapi/linux/tcp.h
> >>>> @@ -353,5 +353,9 @@ struct tcp_zerocopy_receive {
> >>>> __u64 copybuf_address; /* in: copybuf address (small reads) */
> >>>> __s32 copybuf_len; /* in/out: copybuf bytes avail/used or error */
> >>>> __u32 flags; /* in: flags */
> >>>> + __u64 msg_control; /* ancillary data */
> >>>> + __u64 msg_controllen;
> >>>> + __u32 msg_flags;
> >>>> + /* __u32 hole; Next we must add >1 u32 otherwise length checks fail. */
> >>>
> >>> Well, let's hope nobody steps on this landmine.. :)
> >>>
> >>
> >> Past suggestions were made to use anonymous declarations - e.g., __u32
> >> :32; - as a way of reserving the space for future use. That or declare
> >> '__u32 resvd', check that it must be 0 and makes it available for later
> >> (either directly or with a union).
> >
> > This is the schema (reserved field without union) used by the RDMA UAPIs from
> > the beginning (>20 years already) and it works like a charm.
> >
> > Highly recommend :).
> >
>
> agreed.
>
> Arjun: would you mind following up with a patch to make this hole
> explicit and usable for the next extension? Thanks,
Will do.
-Arjun
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy.
2021-02-02 2:20 ` Arjun Roy
@ 2021-02-02 6:52 ` Leon Romanovsky
2021-02-04 23:03 ` Arjun Roy
0 siblings, 1 reply; 12+ messages in thread
From: Leon Romanovsky @ 2021-02-02 6:52 UTC (permalink / raw)
To: Arjun Roy
Cc: David Ahern, Jakub Kicinski, Arjun Roy, David Miller, netdev,
Eric Dumazet, Soheil Hassas Yeganeh
On Mon, Feb 01, 2021 at 06:20:23PM -0800, Arjun Roy wrote:
> On Mon, Feb 1, 2021 at 6:06 PM David Ahern <dsahern@gmail.com> wrote:
> >
> > On 1/24/21 11:15 PM, Leon Romanovsky wrote:
> > > On Fri, Jan 22, 2021 at 10:55:45PM -0700, David Ahern wrote:
> > >> On 1/22/21 9:07 PM, Jakub Kicinski wrote:
> > >>> On Wed, 20 Jan 2021 16:41:48 -0800 Arjun Roy wrote:
> > >>>> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> > >>>> index 768e93bd5b51..b216270105af 100644
> > >>>> --- a/include/uapi/linux/tcp.h
> > >>>> +++ b/include/uapi/linux/tcp.h
> > >>>> @@ -353,5 +353,9 @@ struct tcp_zerocopy_receive {
> > >>>> __u64 copybuf_address; /* in: copybuf address (small reads) */
> > >>>> __s32 copybuf_len; /* in/out: copybuf bytes avail/used or error */
> > >>>> __u32 flags; /* in: flags */
> > >>>> + __u64 msg_control; /* ancillary data */
> > >>>> + __u64 msg_controllen;
> > >>>> + __u32 msg_flags;
> > >>>> + /* __u32 hole; Next we must add >1 u32 otherwise length checks fail. */
> > >>>
> > >>> Well, let's hope nobody steps on this landmine.. :)
> > >>>
> > >>
> > >> Past suggestions were made to use anonymous declarations - e.g., __u32
> > >> :32; - as a way of reserving the space for future use. That or declare
> > >> '__u32 resvd', check that it must be 0 and makes it available for later
> > >> (either directly or with a union).
> > >
> > > This is the schema (reserved field without union) used by the RDMA UAPIs from
> > > the beginning (>20 years already) and it works like a charm.
> > >
> > > Highly recommend :).
> > >
> >
> > agreed.
> >
> > Arjun: would you mind following up with a patch to make this hole
> > explicit and usable for the next extension? Thanks,
>
> Will do.
Please pay attention that all "in" and "out" fields that marked as reserved
should be zeroed and kernel must check "in" field to ensure future compatibility.
Thanks
>
> -Arjun
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy.
2021-02-02 6:52 ` Leon Romanovsky
@ 2021-02-04 23:03 ` Arjun Roy
2021-02-05 0:00 ` Jakub Kicinski
0 siblings, 1 reply; 12+ messages in thread
From: Arjun Roy @ 2021-02-04 23:03 UTC (permalink / raw)
To: Leon Romanovsky
Cc: David Ahern, Jakub Kicinski, Arjun Roy, David Miller, netdev,
Eric Dumazet, Soheil Hassas Yeganeh
On Mon, Feb 1, 2021 at 10:52 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Mon, Feb 01, 2021 at 06:20:23PM -0800, Arjun Roy wrote:
> > On Mon, Feb 1, 2021 at 6:06 PM David Ahern <dsahern@gmail.com> wrote:
> > >
> > > On 1/24/21 11:15 PM, Leon Romanovsky wrote:
> > > > On Fri, Jan 22, 2021 at 10:55:45PM -0700, David Ahern wrote:
> > > >> On 1/22/21 9:07 PM, Jakub Kicinski wrote:
> > > >>> On Wed, 20 Jan 2021 16:41:48 -0800 Arjun Roy wrote:
> > > >>>> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> > > >>>> index 768e93bd5b51..b216270105af 100644
> > > >>>> --- a/include/uapi/linux/tcp.h
> > > >>>> +++ b/include/uapi/linux/tcp.h
> > > >>>> @@ -353,5 +353,9 @@ struct tcp_zerocopy_receive {
> > > >>>> __u64 copybuf_address; /* in: copybuf address (small reads) */
> > > >>>> __s32 copybuf_len; /* in/out: copybuf bytes avail/used or error */
> > > >>>> __u32 flags; /* in: flags */
> > > >>>> + __u64 msg_control; /* ancillary data */
> > > >>>> + __u64 msg_controllen;
> > > >>>> + __u32 msg_flags;
> > > >>>> + /* __u32 hole; Next we must add >1 u32 otherwise length checks fail. */
> > > >>>
> > > >>> Well, let's hope nobody steps on this landmine.. :)
> > > >>>
> > > >>
> > > >> Past suggestions were made to use anonymous declarations - e.g., __u32
> > > >> :32; - as a way of reserving the space for future use. That or declare
> > > >> '__u32 resvd', check that it must be 0 and makes it available for later
> > > >> (either directly or with a union).
> > > >
> > > > This is the schema (reserved field without union) used by the RDMA UAPIs from
> > > > the beginning (>20 years already) and it works like a charm.
> > > >
> > > > Highly recommend :).
> > > >
> > >
> > > agreed.
> > >
> > > Arjun: would you mind following up with a patch to make this hole
> > > explicit and usable for the next extension? Thanks,
> >
> > Will do.
>
> Please pay attention that all "in" and "out" fields that marked as reserved
> should be zeroed and kernel must check "in" field to ensure future compatibility.
>
> Thanks
>
A question about the approach where we mandate it as a reserved field;
assuming in the future it is only used as an OUT field where 0 is a
meaningful no-op value, then just setting it to 0 works just fine.
But, if it's an IN or IN-OUT field, it seems like mandating that the
application set it to 0 could break the case where a future
application sets it to some non-zero value and runs on an older
kernel. And allowing it to be non-zero can maybe yield an unexpected
outcome if an old application that did not zero it runs on a newer
kernel.
So: maybe the right move is to mark it as reserved, not care what the
input value is, always set it to 0 before returning to the user, and
explicitly mandate that any future use of the field must be as an
OUT-only parameter?
Thanks,
-Arjun
> >
> > -Arjun
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy.
2021-02-04 23:03 ` Arjun Roy
@ 2021-02-05 0:00 ` Jakub Kicinski
2021-02-05 1:32 ` Arjun Roy
0 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2021-02-05 0:00 UTC (permalink / raw)
To: Arjun Roy
Cc: Leon Romanovsky, David Ahern, Arjun Roy, David Miller, netdev,
Eric Dumazet, Soheil Hassas Yeganeh
On Thu, 4 Feb 2021 15:03:40 -0800 Arjun Roy wrote:
> But, if it's an IN or IN-OUT field, it seems like mandating that the
> application set it to 0 could break the case where a future
> application sets it to some non-zero value and runs on an older
> kernel.
That usually works fine in practice, 0 means "do what old kernels did /
feature not requested", then if newer userspace sets the field to non-0
that means it requires a feature the kernel doesn't support. So -EINVAL
/ -EOPNOTSUPP is right. BPF syscall has been successfully doing this
since day 1, I'm not aware of any major snags.
> And allowing it to be non-zero can maybe yield an unexpected
> outcome if an old application that did not zero it runs on a newer
> kernel.
Could you refresh our memory as to why we can't require the application
to pass zero-ed memory to TCP ZC? In practice is there are max
reasonable length of the argument that such legacy application may pass
so that we can start checking at a certain offset?
> So: maybe the right move is to mark it as reserved, not care what the
> input value is, always set it to 0 before returning to the user, and
> explicitly mandate that any future use of the field must be as an
> OUT-only parameter?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy.
2021-02-05 0:00 ` Jakub Kicinski
@ 2021-02-05 1:32 ` Arjun Roy
0 siblings, 0 replies; 12+ messages in thread
From: Arjun Roy @ 2021-02-05 1:32 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Leon Romanovsky, David Ahern, Arjun Roy, David Miller, netdev,
Eric Dumazet, Soheil Hassas Yeganeh
On Thu, Feb 4, 2021 at 4:00 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 4 Feb 2021 15:03:40 -0800 Arjun Roy wrote:
> > But, if it's an IN or IN-OUT field, it seems like mandating that the
> > application set it to 0 could break the case where a future
> > application sets it to some non-zero value and runs on an older
> > kernel.
>
> That usually works fine in practice, 0 means "do what old kernels did /
> feature not requested", then if newer userspace sets the field to non-0
> that means it requires a feature the kernel doesn't support. So -EINVAL
> / -EOPNOTSUPP is right. BPF syscall has been successfully doing this
> since day 1, I'm not aware of any major snags.
>
Alright, sounds good.
> > And allowing it to be non-zero can maybe yield an unexpected
> > outcome if an old application that did not zero it runs on a newer
> > kernel.
>
> Could you refresh our memory as to why we can't require the application
> to pass zero-ed memory to TCP ZC? In practice is there are max
> reasonable length of the argument that such legacy application may pass
> so that we can start checking at a certain offset?
>
Actually I think that's fine. We have hitherto been just using length
checks to distinguish between feature capability for rx. zerocopy but
I think we can draw the line at this point (now that there's
ambiguity) and start requiring zero'd memory.
I will send out a patch soon; reserved u32 field, must be set to 0 for
the current kernel, can be non-zero and in/out in future kernels as
discussed.
Thanks,
-Arjun
> > So: maybe the right move is to mark it as reserved, not care what the
> > input value is, always set it to 0 before returning to the user, and
> > explicitly mandate that any future use of the field must be as an
> > OUT-only parameter?
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-02-05 1:33 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-21 0:41 [net-next v2 0/2] tcp: add CMSG+rx timestamps to rx. zerocopy Arjun Roy
2021-01-21 0:41 ` [net-next v2 1/2] tcp: Remove CMSG magic numbers for tcp_recvmsg() Arjun Roy
2021-01-21 0:41 ` [net-next v2 2/2] tcp: Add receive timestamp support for receive zerocopy Arjun Roy
2021-01-23 4:07 ` Jakub Kicinski
2021-01-23 5:55 ` David Ahern
2021-01-25 6:15 ` Leon Romanovsky
2021-02-02 2:06 ` David Ahern
2021-02-02 2:20 ` Arjun Roy
2021-02-02 6:52 ` Leon Romanovsky
2021-02-04 23:03 ` Arjun Roy
2021-02-05 0:00 ` Jakub Kicinski
2021-02-05 1:32 ` Arjun Roy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).