2024-03-13 23:51:00

by Abhishek Chauhan (ABC)

[permalink] [raw]
Subject: [PATCH net-next v1] Revert "net: Re-use and set mono_delivery_time bit for userspace tstamp packets"

This reverts commit 30bb896b98fce7d823a96fc02cd69be30384a5cc.

Signed-off-by: Abhishek Chauhan <[email protected]>
---
Reason for revert -
https://lore.kernel.org/netdev/[email protected]/
1. Fallback to design proposed in version 1 to have a seperate user_delivery_time
bit
2. Keep mono_delivery_time as ease
3. Add user_delivery_time as a new bitfield will be a seperate patch
4. do not reset the time if either mono_delivery_time or user_delivery_time is set
5. we will need a user_delivery_time bit and use that to look at sk_clockid

include/linux/skbuff.h | 6 +++---
net/ipv4/ip_output.c | 1 -
net/ipv4/raw.c | 1 -
net/ipv6/ip6_output.c | 2 +-
net/ipv6/raw.c | 2 +-
net/packet/af_packet.c | 4 +---
6 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 4726298d4ed4..2dde34c29203 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -817,9 +817,9 @@ typedef unsigned char *sk_buff_data_t;
* @decrypted: Decrypted SKB
* @slow_gro: state present at GRO time, slower prepare step required
* @mono_delivery_time: When set, skb->tstamp has the
- * delivery_time in mono clock base (i.e., EDT) or a clock base chosen
- * by SO_TXTIME. If zero, skb->tstamp has the (rcv) timestamp at
- * ingress.
+ * delivery_time in mono clock base (i.e. EDT). Otherwise, the
+ * skb->tstamp has the (rcv) timestamp at ingress and
+ * delivery_time at egress.
* @napi_id: id of the NAPI struct this skb came from
* @sender_cpu: (aka @napi_id) source CPU in XPS
* @alloc_cpu: CPU which did the skb allocation.
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index ff1df64c5697..5b5a0adb927f 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1455,7 +1455,6 @@ struct sk_buff *__ip_make_skb(struct sock *sk,
skb->priority = (cork->tos != -1) ? cork->priority: READ_ONCE(sk->sk_priority);
skb->mark = cork->mark;
skb->tstamp = cork->transmit_time;
- skb->mono_delivery_time = !!skb->tstamp;
/*
* Steal rt from cork.dst to avoid a pair of atomic_inc/atomic_dec
* on dst refcount
diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index c4c29fc5b73f..aea89326c697 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -353,7 +353,6 @@ static int raw_send_hdrinc(struct sock *sk, struct flowi4 *fl4,
skb->priority = READ_ONCE(sk->sk_priority);
skb->mark = sockc->mark;
skb->tstamp = sockc->transmit_time;
- skb->mono_delivery_time = !!skb->tstamp;
skb_dst_set(skb, &rt->dst);
*rtp = NULL;

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 2fc1d03dc07d..a722a43dd668 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1922,7 +1922,7 @@ struct sk_buff *__ip6_make_skb(struct sock *sk,
skb->priority = READ_ONCE(sk->sk_priority);
skb->mark = cork->base.mark;
skb->tstamp = cork->base.transmit_time;
- skb->mono_delivery_time = !!skb->tstamp;
+
ip6_cork_steal_dst(skb, cork);
IP6_INC_STATS(net, rt->rt6i_idev, IPSTATS_MIB_OUTREQUESTS);
if (proto == IPPROTO_ICMPV6) {
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 13f54f8eea35..03dbb874c363 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -616,7 +616,7 @@ static int rawv6_send_hdrinc(struct sock *sk, struct msghdr *msg, int length,
skb->priority = READ_ONCE(sk->sk_priority);
skb->mark = sockc->mark;
skb->tstamp = sockc->transmit_time;
- skb->mono_delivery_time = !!skb->tstamp;
+
skb_put(skb, length);
skb_reset_network_header(skb);
iph = ipv6_hdr(skb);
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 0db31ca4982d..c9bbc2686690 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2057,7 +2057,7 @@ static int packet_sendmsg_spkt(struct socket *sock, struct msghdr *msg,
skb->priority = READ_ONCE(sk->sk_priority);
skb->mark = READ_ONCE(sk->sk_mark);
skb->tstamp = sockc.transmit_time;
- skb->mono_delivery_time = !!skb->tstamp;
+
skb_setup_tx_timestamp(skb, sockc.tsflags);

if (unlikely(extra_len == 4))
@@ -2586,7 +2586,6 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
skb->priority = READ_ONCE(po->sk.sk_priority);
skb->mark = READ_ONCE(po->sk.sk_mark);
skb->tstamp = sockc->transmit_time;
- skb->mono_delivery_time = !!skb->tstamp;
skb_setup_tx_timestamp(skb, sockc->tsflags);
skb_zcopy_set_nouarg(skb, ph.raw);

@@ -3065,7 +3064,6 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
skb->priority = READ_ONCE(sk->sk_priority);
skb->mark = sockc.mark;
skb->tstamp = sockc.transmit_time;
- skb->mono_delivery_time = !!skb->tstamp;

if (unlikely(extra_len == 4))
skb->no_fcs = 1;
--
2.25.1



2024-03-14 00:16:36

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH net-next v1] Revert "net: Re-use and set mono_delivery_time bit for userspace tstamp packets"

On Wed, 13 Mar 2024 16:50:03 -0700 Abhishek Chauhan wrote:
> Subject: [PATCH net-next v1] Revert "net: Re-use and set mono_delivery_time bit for userspace tstamp packets"
> Date: Wed, 13 Mar 2024 16:50:03 -0700
> X-Mailer: git-send-email 2.25.1
>
> This reverts commit 30bb896b98fce7d823a96fc02cd69be30384a5cc.
>
> Signed-off-by: Abhishek Chauhan <[email protected]>
> ---
> Reason for revert -
> https://lore.kernel.org/netdev/[email protected]/

> 1. Fallback to design proposed in version 1 to have a seperate user_delivery_time
> bit
> 2. Keep mono_delivery_time as ease
> 3. Add user_delivery_time as a new bitfield will be a seperate patch
> 4. do not reset the time if either mono_delivery_time or user_delivery_time is set
> 5. we will need a user_delivery_time bit and use that to look at sk_clockid

Could you please spell this out a bit more in the commit message and
add the link as a:

Link: https://lore.kernel.org/netdev/[email protected]/

in the commit message rather than the "throw away notes" ?

(feel free to repost asap, without usual netdev's 24h)
--
pw-bot: cr

2024-03-14 00:37:55

by Abhishek Chauhan (ABC)

[permalink] [raw]
Subject: Re: [PATCH net-next v1] Revert "net: Re-use and set mono_delivery_time bit for userspace tstamp packets"



On 3/13/2024 5:16 PM, Jakub Kicinski wrote:
> On Wed, 13 Mar 2024 16:50:03 -0700 Abhishek Chauhan wrote:
>> Subject: [PATCH net-next v1] Revert "net: Re-use and set mono_delivery_time bit for userspace tstamp packets"
>> Date: Wed, 13 Mar 2024 16:50:03 -0700
>> X-Mailer: git-send-email 2.25.1
>>
>> This reverts commit 30bb896b98fce7d823a96fc02cd69be30384a5cc.
>>
>> Signed-off-by: Abhishek Chauhan <[email protected]>
>> ---
>> Reason for revert -
>> https://lore.kernel.org/netdev/[email protected]/
>
>> 1. Fallback to design proposed in version 1 to have a seperate user_delivery_time
>> bit
>> 2. Keep mono_delivery_time as ease
>> 3. Add user_delivery_time as a new bitfield will be a seperate patch
>> 4. do not reset the time if either mono_delivery_time or user_delivery_time is set
>> 5. we will need a user_delivery_time bit and use that to look at sk_clockid
>
> Could you please spell this out a bit more in the commit message and
> add the link as a:
>

Certainly i can do that and explain more in the Revert commit message and point a link as mentioned
below as why we need to fall back to design 1 proposal for adding an extra bit.
> Link: https://lore.kernel.org/netdev/[email protected]/
>
> in the commit message rather than the "throw away notes" ?
>
> (feel free to repost asap, without usual netdev's 24h)