Hello Matthieu, On Mon, Apr 22, 2024 at 4:47 PM Matthieu Baerts <matt...@kernel.org> wrote: > > Hi Jason, > > On 22/04/2024 05:01, Jason Xing wrote: > > From: Jason Xing <kernelx...@tencent.com> > > > > Add a new standalone file for the easy future extension to support > > both active reset and passive reset in the TCP/DCCP/MPTCP protocols. > > Thank you for looking at that!
Thanks for the review! > > (...) > > > diff --git a/include/net/rstreason.h b/include/net/rstreason.h > > new file mode 100644 > > index 000000000000..c57bc5413c17 > > --- /dev/null > > +++ b/include/net/rstreason.h > > @@ -0,0 +1,144 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > + > > +#ifndef _LINUX_RSTREASON_H > > +#define _LINUX_RSTREASON_H > > +#include <net/dropreason-core.h> > > +#include <uapi/linux/mptcp.h> > > + > > +#define DEFINE_RST_REASON(FN, FNe) \ > > + FN(MPTCP_RST_EUNSPEC) \ > > + FN(MPTCP_RST_EMPTCP) \ > > + FN(MPTCP_RST_ERESOURCE) \ > > + FN(MPTCP_RST_EPROHIBIT) \ > > + FN(MPTCP_RST_EWQ2BIG) \ > > + FN(MPTCP_RST_EBADPERF) \ > > + FN(MPTCP_RST_EMIDDLEBOX) \ > > Small detail: should it not make more sense to put the ones linked to > MPTCP at the end? I mean I guess MPTCP should be treated in second > priority: CONFIG_MPTCP could not be set, and the ones linked to TCP > should be more frequent, etc. Do you mean that I need to adjust the order: 1) tcp reasons first, 2) independent reasons, 3) mptcp reasons ? Reasonable. I will do it :) > > > + FN(NOT_SPECIFIED) \ > > + FN(NO_SOCKET) \ > > + FNe(MAX) > > (...) > > > +/* Convert reset reasons in MPTCP to our own enum type */ > > +static inline enum sk_rst_reason convert_mptcpreason(u32 reason) > > +{ > > + switch (reason) { > > + case MPTCP_RST_EUNSPEC: > > + return SK_RST_REASON_MPTCP_RST_EUNSPEC; > > + case MPTCP_RST_EMPTCP: > > + return SK_RST_REASON_MPTCP_RST_EMPTCP; > > + case MPTCP_RST_ERESOURCE: > > + return SK_RST_REASON_MPTCP_RST_ERESOURCE; > > + case MPTCP_RST_EPROHIBIT: > > + return SK_RST_REASON_MPTCP_RST_EPROHIBIT; > > + case MPTCP_RST_EWQ2BIG: > > + return SK_RST_REASON_MPTCP_RST_EWQ2BIG; > > + case MPTCP_RST_EBADPERF: > > + return SK_RST_REASON_MPTCP_RST_EBADPERF; > > + case MPTCP_RST_EMIDDLEBOX: > > + return SK_RST_REASON_MPTCP_RST_EMIDDLEBOX; > > + default: > > + /** > > + * It should not happen, or else errors may occur > > + * in MPTCP layer > > + */ > > + return SK_RST_REASON_ERROR; > > + } > > +} > > If this helper is only used on MPTCP, maybe better to move it to > net/mptcp/protocol.h (and to patch 5/7?)? We tried to isolate MPTCP code. Roger that. I will move the helper into protocol.h as well as the patch itself. > > Also, maybe it is just me, but I'm not a big fan of the helper name: > convert_mptcpreason() (same for the "drop" one). I think it should at > least mention its "origin" (rst reason): e.g. something like > (sk_)rst_reason_convert_mptcp or (sk_)rst_convert_mptcp_reason() (or > mptcp_to_rst_reason())? > > And (sk_)rst_reason_convert_(skb_)drop() (or skb_drop_to_rst_reason())? I agree with you. Actually I had a local patch where I used sk_rst_reason_skbdrop() and sk_rst_reason_mptcpreason(). Interestingly, I changed them in this patch series due to the function name being too long (which is my initial thought). I will use sk_rst_convert_xxx_reason() as you suggested. > > > +/* Convert reset reasons in MPTCP to our own enum type */ > > I don't think this part is linked to MPTCP, right? Ah, copy-paste syndrome... Sorry, I will correct it. > > > +static inline enum sk_rst_reason convert_dropreason(enum skb_drop_reason > > reason) > > +{ > > + switch (reason) { > > + case SKB_DROP_REASON_NOT_SPECIFIED: > > + return SK_RST_REASON_NOT_SPECIFIED; > > + case SKB_DROP_REASON_NO_SOCKET: > > + return SK_RST_REASON_NO_SOCKET; > > + default: > > + /* If we don't have our own corresponding reason */ > > + return SK_RST_REASON_NOT_SPECIFIED; > > + } > > +} > > (This helper could be introduced in patch 4/7 because it is not used > before, but I'm fine either ways.) Good. It makes more sense. Thanks, Jason