| Message ID | 20220128073319.1017084-2-imagedong@tencent.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: use kfree_skb_reason() for ip/udp packet receive | expand |
On 1/28/22 12:33 AM, menglong8.dong@gmail.com wrote: > From: Menglong Dong <imagedong@tencent.com> > > Add document for following existing drop reasons: > > SKB_DROP_REASON_NOT_SPECIFIED > SKB_DROP_REASON_NO_SOCKET > SKB_DROP_REASON_PKT_TOO_SMALL > SKB_DROP_REASON_TCP_CSUM > SKB_DROP_REASON_SOCKET_FILTER > SKB_DROP_REASON_UDP_CSUM > > Signed-off-by: Menglong Dong <imagedong@tencent.com> > --- > include/linux/skbuff.h | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > Reviewed-by: David Ahern <dsahern@kernel.org>
Hello! On Tue, Feb 1, 2022 at 1:14 AM David Ahern <dsahern@gmail.com> wrote: > > On 1/28/22 12:33 AM, menglong8.dong@gmail.com wrote: > > From: Menglong Dong <imagedong@tencent.com> > > > > Add document for following existing drop reasons: > > > > SKB_DROP_REASON_NOT_SPECIFIED > > SKB_DROP_REASON_NO_SOCKET > > SKB_DROP_REASON_PKT_TOO_SMALL > > SKB_DROP_REASON_TCP_CSUM > > SKB_DROP_REASON_SOCKET_FILTER > > SKB_DROP_REASON_UDP_CSUM > > > > Signed-off-by: Menglong Dong <imagedong@tencent.com> > > --- > > include/linux/skbuff.h | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > Reviewed-by: David Ahern <dsahern@kernel.org> > > I'm doing the job of using kfree_skb_reason() for the TCP layer, and I have some puzzles. When collecting drop reason for tcp_v4_inbound_md5_hash() in tcp_v4_rcv(), I come up with 2 ways: First way: pass the address of reason to tcp_v4_inbound_md5_hash() like this: static bool tcp_v4_inbound_md5_hash(const struct sock *sk, const struct sk_buff *skb, - int dif, int sdif) + int dif, int sdif, + enum skb_drop_reason *reason) This can work, but many functions like tcp_v4_inbound_md5_hash() need to do such a change. Second way: introduce a 'drop_reason' field to 'struct sk_buff'. Therefore, drop reason can be set by 'skb->drop_reason = SKB_DROP_REASON_XXX' anywhere. For TCP, there are many cases where you can't get a drop reason in the place where skb is freed, so I think there needs to be a way to deeply collect drop reasons. The second can resolve this problem easily, but extra fields may have performance problems. Do you have some better ideas? Thanks! Menglong Dong
On Thu, 10 Feb 2022 11:19:49 +0800 Menglong Dong wrote: > I'm doing the job of using kfree_skb_reason() for the TCP layer, > and I have some puzzles. > > When collecting drop reason for tcp_v4_inbound_md5_hash() in > tcp_v4_rcv(), I come up with 2 ways: > > First way: pass the address of reason to tcp_v4_inbound_md5_hash() > like this: > > static bool tcp_v4_inbound_md5_hash(const struct sock *sk, > const struct sk_buff *skb, > - int dif, int sdif) > + int dif, int sdif, > + enum skb_drop_reason *reason) > > This can work, but many functions like tcp_v4_inbound_md5_hash() > need to do such a change. > > Second way: introduce a 'drop_reason' field to 'struct sk_buff'. Therefore, > drop reason can be set by 'skb->drop_reason = SKB_DROP_REASON_XXX' > anywhere. > > For TCP, there are many cases where you can't get a drop reason in > the place where skb is freed, so I think there needs to be a way to > deeply collect drop reasons. The second can resolve this problem > easily, but extra fields may have performance problems. > > Do you have some better ideas? On a quick look tcp_v4_inbound_md5_hash() returns a drop / no drop decision, so you could just change the return type to enum skb_drop_reason. SKB_DROP_REASON_NOT_SPECIFIED is 0 is false, so if (reason) goto drop; logic will hold up.
On Thu, Feb 10, 2022 at 1:12 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 10 Feb 2022 11:19:49 +0800 Menglong Dong wrote: > > I'm doing the job of using kfree_skb_reason() for the TCP layer, > > and I have some puzzles. > > > > When collecting drop reason for tcp_v4_inbound_md5_hash() in > > tcp_v4_rcv(), I come up with 2 ways: > > > > First way: pass the address of reason to tcp_v4_inbound_md5_hash() > > like this: > > > > static bool tcp_v4_inbound_md5_hash(const struct sock *sk, > > const struct sk_buff *skb, > > - int dif, int sdif) > > + int dif, int sdif, > > + enum skb_drop_reason *reason) > > > > This can work, but many functions like tcp_v4_inbound_md5_hash() > > need to do such a change. > > > > Second way: introduce a 'drop_reason' field to 'struct sk_buff'. Therefore, > > drop reason can be set by 'skb->drop_reason = SKB_DROP_REASON_XXX' > > anywhere. > > > > For TCP, there are many cases where you can't get a drop reason in > > the place where skb is freed, so I think there needs to be a way to > > deeply collect drop reasons. The second can resolve this problem > > easily, but extra fields may have performance problems. > > > > Do you have some better ideas? > > On a quick look tcp_v4_inbound_md5_hash() returns a drop / no drop > decision, so you could just change the return type to enum > skb_drop_reason. SKB_DROP_REASON_NOT_SPECIFIED is 0 is false, > so if (reason) goto drop; logic will hold up. Yeah, that's an idea. But some functions are more complex, such as tcp_rcv_state_process() and tcp_rcv_state_process()->tcp_v4_conn_request(). The return value of tcp_rcv_state_process() can't be reused, and it's hard to add a function param of type 'enum skb_drop_reason *' to tcp_v4_conn_request(). There are some nice drop reasons in tcp_v4_conn_request(), it's a pity to give up them. How about introducing a field to 'struct sock' for drop reasons? As sk is locked during the packet process in tcp_v4_do_rcv(), this seems to work. Thanks! Menglong Dong
On Thu, 10 Feb 2022 21:42:14 +0800 Menglong Dong wrote: > How about introducing a field to 'struct sock' for drop reasons? As sk is > locked during the packet process in tcp_v4_do_rcv(), this seems to work. I find adding temporary storage to persistent data structures awkward. You can put a structure on the stack and pass it thru the call chain, that's just my subjective preference, tho, others may have better ideas.
On Thu, Feb 10, 2022 at 8:13 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 10 Feb 2022 21:42:14 +0800 Menglong Dong wrote: > > How about introducing a field to 'struct sock' for drop reasons? As sk is > > locked during the packet process in tcp_v4_do_rcv(), this seems to work. > > I find adding temporary storage to persistent data structures awkward. > You can put a structure on the stack and pass it thru the call chain, > that's just my subjective preference, tho, others may have better ideas. I had a similar TODO item, because stuff like 'waking up task' or free one skb (or list of skb) could be performed outside of socket lock.
On Fri, Feb 11, 2022 at 12:13 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 10 Feb 2022 21:42:14 +0800 Menglong Dong wrote: > > How about introducing a field to 'struct sock' for drop reasons? As sk is > > locked during the packet process in tcp_v4_do_rcv(), this seems to work. > > I find adding temporary storage to persistent data structures awkward. > You can put a structure on the stack and pass it thru the call chain, > that's just my subjective preference, tho, others may have better ideas. Yes, I also feel it is awkward. I'll try to do this job by passing drop reasons through the call chain. Thanks for your help :)
On Fri, Feb 11, 2022 at 12:29 AM Eric Dumazet <edumazet@google.com> wrote: > > On Thu, Feb 10, 2022 at 8:13 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Thu, 10 Feb 2022 21:42:14 +0800 Menglong Dong wrote: > > > How about introducing a field to 'struct sock' for drop reasons? As sk is > > > locked during the packet process in tcp_v4_do_rcv(), this seems to work. > > > > I find adding temporary storage to persistent data structures awkward. > > You can put a structure on the stack and pass it thru the call chain, > > that's just my subjective preference, tho, others may have better ideas. > > I had a similar TODO item, because stuff like 'waking up task' or free > one skb (or list of skb) could be performed outside of socket lock. May I ask what it's like? Is it used to solve this kind of problem? Thanks! Menglong Dong
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 8a636e678902..5c5615a487e7 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -314,12 +314,12 @@ struct sk_buff; * used to translate the reason to string. */ enum skb_drop_reason { - SKB_DROP_REASON_NOT_SPECIFIED, - SKB_DROP_REASON_NO_SOCKET, - SKB_DROP_REASON_PKT_TOO_SMALL, - SKB_DROP_REASON_TCP_CSUM, - SKB_DROP_REASON_SOCKET_FILTER, - SKB_DROP_REASON_UDP_CSUM, + SKB_DROP_REASON_NOT_SPECIFIED, /* drop reason is not specified */ + SKB_DROP_REASON_NO_SOCKET, /* socket not found */ + SKB_DROP_REASON_PKT_TOO_SMALL, /* packet size is too small */ + SKB_DROP_REASON_TCP_CSUM, /* TCP checksum error */ + SKB_DROP_REASON_SOCKET_FILTER, /* dropped by socket filter */ + SKB_DROP_REASON_UDP_CSUM, /* UDP checksum error */ SKB_DROP_REASON_MAX, };