On Fri, Apr 19, 2024 at 9:29 AM Jason Xing <kerneljasonx...@gmail.com> wrote: > > On Fri, Apr 19, 2024 at 3:02 PM Eric Dumazet <eduma...@google.com> wrote: > > > > On Fri, Apr 19, 2024 at 4:31 AM Jason Xing <kerneljasonx...@gmail.com> > > wrote: > > > > > > On Fri, Apr 19, 2024 at 7:26 AM Jason Xing <kerneljasonx...@gmail.com> > > > wrote: > > > > > > > > > When I said "If you feel the need to put them in a special group, this > > > > > is fine by me.", > > > > > this was really about partitioning the existing enum into groups, if > > > > > you prefer having a group of 'RES reasons' > > > > > > > > Are you suggesting copying what we need from enum skb_drop_reason{} to > > > > enum sk_rst_reason{}? Why not reusing them directly. I have no idea > > > > what the side effect of cast conversion itself is? > > > > > > Sorry that I'm writing this email. I'm worried my statement is not > > > that clear, so I write one simple snippet which can help me explain > > > well :) > > > > > > Allow me give NO_SOCKET as an example: > > > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c > > > index e63a3bf99617..2c9f7364de45 100644 > > > --- a/net/ipv4/icmp.c > > > +++ b/net/ipv4/icmp.c > > > @@ -767,6 +767,7 @@ void __icmp_send(struct sk_buff *skb_in, int type, > > > int code, __be32 info, > > > if (!fl4.saddr) > > > fl4.saddr = htonl(INADDR_DUMMY); > > > > > > + trace_icmp_send(skb_in, type, code); > > > icmp_push_reply(sk, &icmp_param, &fl4, &ipc, &rt); > > > ende: > > > ip_rt_put(rt); > > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > > > index 1e650ec71d2f..d5963831280f 100644 > > > --- a/net/ipv4/tcp_ipv4.c > > > +++ b/net/ipv4/tcp_ipv4.c > > > @@ -2160,6 +2160,7 @@ int tcp_v4_rcv(struct sk_buff *skb) > > > { > > > struct net *net = dev_net(skb->dev); > > > enum skb_drop_reason drop_reason; > > > + enum sk_rst_reason rst_reason; > > > int sdif = inet_sdif(skb); > > > int dif = inet_iif(skb); > > > const struct iphdr *iph; > > > @@ -2355,7 +2356,8 @@ int tcp_v4_rcv(struct sk_buff *skb) > > > bad_packet: > > > __TCP_INC_STATS(net, TCP_MIB_INERRS); > > > } else { > > > - tcp_v4_send_reset(NULL, skb); > > > + rst_reason = RST_REASON_NO_SOCKET; > > > + tcp_v4_send_reset(NULL, skb, rst_reason); > > > } > > > > > > discard_it: > > > > > > As you can see, we need to add a new 'rst_reason' variable which > > > actually is the same as drop reason. They are the same except for the > > > enum type... Such rst_reasons/drop_reasons are all over the place. > > > > > > Eric, if you have a strong preference, I can do it as you said. > > > > > > Well, how about explicitly casting them like this based on the current > > > series. It looks better and clearer and more helpful to people who is > > > reading codes to understand: > > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > > > index 461b4d2b7cfe..eb125163d819 100644 > > > --- a/net/ipv4/tcp_ipv4.c > > > +++ b/net/ipv4/tcp_ipv4.c > > > @@ -1936,7 +1936,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff > > > *skb) > > > return 0; > > > > > > reset: > > > - tcp_v4_send_reset(rsk, skb, (u32)reason); > > > + tcp_v4_send_reset(rsk, skb, (enum sk_rst_reason)reason); > > > discard: > > > kfree_skb_reason(skb, reason); > > > /* Be careful here. If this function gets more complicated and > > > > It makes no sense to declare an enum sk_rst_reason and then convert it > > to drop_reason > > or vice versa. > > > > Next thing you know, compiler guys will add a new -Woption that will > > forbid such conversions. > > > > Please add to tcp_v4_send_reset() an skb_drop_reason, not a new enum. > > Ah... It looks like I didn't make myself clear again. Sorry... > Actually I wrote this part many times. My conclusion is that It's not > feasible to do this. > > REASONS: > If we __only__ need to deal with this passive reset in TCP, it's fine. > We pass a skb_drop_reason which should also be converted to u32 type > in tcp_v4_send_reset() as you said, it can work. People who use the > trace will see the reason with the 'SKB_DROP_REASON' prefix stripped. > > But we have to deal with other cases. A few questions are listed here: > 1) What about tcp_send_active_reset() in TCP/MPTCP? Passing weird drop > reasons? There is no drop reason at all. I think people will get > confused. So I believe we should invent new definitions to cope with > it. > 2) What about the .send_reset callback in the subflow_syn_recv_sock() > in MPTCP? The reasons in MPTCP are only definitions (such as > MPTCP_RST_EUNSPEC). I don't think we can convert them into the enum > skb_drop_reason type. > > So where should we group those various reasons? > > Introducing a new enum is for extension and compatibility for all > kinds of reset reasons. > > What do you think?
I will stop repeating myself. enums are not what you think. type safety is there for a reason. Can you show me another place in networking stacks where we cast enums to others ?