diff mbox series

[net-next,2/9] net: tcp: add 'drop_reason' field to struct tcp_skb_cb

Message ID 20221029130957.1292060-3-imagedong@tencent.com
State New
Headers
Series net: tcp: add skb drop reasons to tcp state process |

Commit Message

Menglong Dong Oct. 29, 2022, 1:09 p.m. UTC 
  From: Menglong Dong <imagedong@tencent.com>

Because of the long call chain on processing skb in TCP protocol, it's
hard to pass the drop reason to the code where the skb is freed.

Therefore, we can store the drop reason to skb->cb, and pass it to
kfree_skb_reason(). I'm lucky, the struct tcp_skb_cb still has 4 bytes
spare space for this purpose.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/net/tcp.h | 3 +++
 1 file changed, 3 insertions(+)

Comments

Eric Dumazet Oct. 29, 2022, 3:23 p.m. UTC | #1 
On Sat, Oct 29, 2022 at 6:11 AM <menglong8.dong@gmail.com> wrote:
>
> From: Menglong Dong <imagedong@tencent.com>
>
> Because of the long call chain on processing skb in TCP protocol, it's
> hard to pass the drop reason to the code where the skb is freed.
>
> Therefore, we can store the drop reason to skb->cb, and pass it to
> kfree_skb_reason(). I'm lucky, the struct tcp_skb_cb still has 4 bytes
> spare space for this purpose.

No, we have needs for this space for future use.

skb->cb[] purpose is to store semi-permanent info, for skbs that stay
in a queue.

Here, you need a state stored only in the context of the current context.
Stack variables are better.
Menglong Dong Oct. 31, 2022, 3:08 a.m. UTC | #2 
Hello,

On Sat, Oct 29, 2022 at 11:23 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Sat, Oct 29, 2022 at 6:11 AM <menglong8.dong@gmail.com> wrote:
> >
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > Because of the long call chain on processing skb in TCP protocol, it's
> > hard to pass the drop reason to the code where the skb is freed.
> >
> > Therefore, we can store the drop reason to skb->cb, and pass it to
> > kfree_skb_reason(). I'm lucky, the struct tcp_skb_cb still has 4 bytes
> > spare space for this purpose.
>
> No, we have needs for this space for future use.
>
> skb->cb[] purpose is to store semi-permanent info, for skbs that stay
> in a queue.
>

May I use it for a while? Or, can I make it a union with
some field, such as 'header'? As the 'drop_reason' field will
be set only if it is going to be freed.

> Here, you need a state stored only in the context of the current context.
> Stack variables are better.

It's hard to get the drop reason through stack variables,
especially some functions in TCP protocol, such as:

  tcp_rcv_synsent_state_process
  tcp_timewait_state_process
  tcp_conn_request
  tcp_rcv_state_process

And it will mess the code a little up, just like what
you comment in this series:

https://lore.kernel.org/netdev/20220617100514.7230-1-imagedong@tencent.com/

I hope to complete this part, or I think I can't move
ahead :/

Thanks!
Menglong Dong
diff mbox series

Patch

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 14d45661a84d..0b6e39ca83b4 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -888,6 +888,7 @@  struct tcp_skb_cb {
 			has_rxtstamp:1,	/* SKB has a RX timestamp	*/
 			unused:5;
 	__u32		ack_seq;	/* Sequence number ACK'd	*/
+	enum skb_drop_reason drop_reason; /* Why skb is dropped */
 	union {
 		struct {
 #define TCPCB_DELIVERED_CE_MASK ((1U<<20) - 1)
@@ -912,6 +913,8 @@  struct tcp_skb_cb {
 };
 
 #define TCP_SKB_CB(__skb)	((struct tcp_skb_cb *)&((__skb)->cb[0]))
+#define TCP_SKB_DR(__skb, reason) \
+	(TCP_SKB_CB(__skb)->drop_reason = SKB_DROP_REASON_##reason)
 
 extern const struct inet_connection_sock_af_ops ipv4_specific;