Hello Steven, On Sat, Apr 20, 2024 at 10:36 AM Steven Rostedt <rost...@goodmis.org> wrote: > > On Fri, 19 Apr 2024 16:00:20 +0800 > Jason Xing <kerneljasonx...@gmail.com> wrote: > > > If other experts see this thread, please help me. I would appreciate > > it. I have strong interests and feel strong responsibility to > > implement something like this patch series. It can be very useful!! > > I'm not a networking expert, but as I'm Cc'd and this is about tracing, > I'll jump in to see if I can help. Honestly, reading the thread, it > appears that you and Eric are talking past each other. > > I believe Eric is concerned about losing the value of the enum. Enums > are types, and if you typecast them to another type, they lose the > previous type, and all the safety that goes with it.
Ah, I see. Possible lost value in another enum could cause a problem. > > Now, I do not really understand the problem trying to be solved here. I > understand how TCP works but I never looked into the implementation of > MPTCP. > > You added this: > > +static inline enum sk_rst_reason convert_mptcp_reason(u32 reason) > +{ > + return reason += RST_REASON_START; > +} > > And used it for places like this: > > @@ -309,8 +309,13 @@ static struct dst_entry *subflow_v4_route_req(const > struct sock *sk, > return dst; > > dst_release(dst); > - if (!req->syncookie) > - tcp_request_sock_ops.send_reset(sk, skb, > SK_RST_REASON_NOT_SPECIFIED); > + if (!req->syncookie) { > + struct mptcp_ext *mpext = mptcp_get_ext(skb); > + enum sk_rst_reason reason; > + > + reason = convert_mptcp_reason(mpext->reset_reason); > + tcp_request_sock_ops.send_reset(sk, skb, reason); > + } > return NULL; > } > > As I don't know this code or how MPTCP works, I do not understand the > above. It use to pass to send_reset() SK_RST_REASON_NOT_SPECIFIED. But > now it takes a "reset_reason" calls the "convert_mptcp_reason()" to get > back a enum value. > > If you are mapping the reset_reason to enum sk_rst_reason, why not do > it via a real conversion instead of this fragile arithmetic between the two > values? > > static inline enum sk_rst_reason convert_mptcp_reason(u32 reason) > { > switch(reason) { > case 0: return SK_RST_REASON_MPTCP_RST_EUNSPEC; > case 1: return SK_RST_REASON_MPTCP_RST_EMPTCP; > case 2: return SK_RST_REASON_MPTCP_RST_ERESOURCE; > [..] > default: return SK_RST_REASON_MAX; // or some other error value > ] > } This code snippet looks much better than mine. > > I'm not sure if this is any better, but it's not doing any casting and > it's easier to understand. It's a simple mapping between the reason and > the enum and there's no inherit dependency between the values. Could > possibly create enums for the reason numbers and replace the hard coded > values with them. Right. I also need to handle many drop reasons cases like what you do. Due to too many of them, I will try the key-value map instead of switch-case and then see if it works. > > That way that helper function is at least doing a real conversion of > one type to another. > > But like I said from the beginning. I don't understand the details here > and have not spent the time to dig deeper. I just read the thread and I > agree with Eric that the arithmetic conversion of reason to an enum > looks fragile at best and buggy at worst. Thanks so much for your help, which I didn't even imagine. Sure, after one night of investigation, I figured it out. I will drop enum casts without any doubt as Eric and you suggested. But I believe a new enum is needed, grouping various reasons together which help ftrace print the valid string to userspace. Thanks, Jason > > -- Steve