From: Geliang Tang <geliangtang@gmail.com>
To: Yonglong Li <liyonglong@chinatelecom.cn>
Cc: mptcp@lists.linux.dev,
Mat Martineau <mathew.j.martineau@linux.intel.com>
Subject: Re: [PATCH v7 4/5] mptcp: remove some double-check
Date: Fri, 2 Jul 2021 16:06:13 +0800 [thread overview]
Message-ID: <CA+WQbwvF_3FMVWjFD4=iOA87Quh6rGZpJ8htMYsexBCcGtNiAQ@mail.gmail.com> (raw)
In-Reply-To: <6771e9d0-102a-a261-fe97-b9104d432683@chinatelecom.cn>
Yonglong Li <liyonglong@chinatelecom.cn> 于2021年7月2日周五 下午2:22写道：
>
> Hi Geliang,
>
> I think these double check is unnecessary. the reason to keep them is?
> I think keep "!mptcp_pm_should_add_signal(msk)" you said in v6 is reasonable,
> It can avoid to get pm.lock in process of sending packets. But the other double
> check is useless.
The length re-check is for the no-spin-lock optimization too.
These code is no harm for yours, why can't you keep it there. :)
-Geliang
>
> On 2021/6/30 18:57, Geliang Tang wrote:
> > As I said in v6, I prefer to keep these double check code, no need to
> > remove them.
> >
> > Yonglong Li <liyonglong@chinatelecom.cn> 于2021年6月30日周三 下午6:24写道：
> >>
> >> remove some double-check in mptcp_established_options_add_addr() and
> >> mptcp_established_options_rm_addr()
> >>
> >> Signed-off-by: Yonglong Li <liyonglong@chinatelecom.cn>
> >> ---
> >> net/mptcp/options.c | 14 ++------------
> >> net/mptcp/pm.c | 21 +++++++++++----------
> >> net/mptcp/protocol.h | 4 ++--
> >> 3 files changed, 15 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> >> index cceff0a..0711fc1 100644
> >> --- a/net/mptcp/options.c
> >> +++ b/net/mptcp/options.c
> >> @@ -659,7 +659,7 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >> int len = 0;
> >>
> >> if (!mptcp_pm_should_add_signal(msk) ||
> >> - !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr))
> >> + !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts, &add_addr, &len))
> >> return false;
> >>
> >> if (((add_addr & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> >> @@ -674,10 +674,6 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >> drop_other_suboptions = true;
> >> }
> >>
> >> - len = mptcp_add_addr_len(msk, opts);
> >> - if (remaining < len)
> >> - return false;
> >> -
> >> *size = len;
> >> if (drop_other_suboptions)
> >> *size -= opt_size;
> >> @@ -707,13 +703,7 @@ static bool mptcp_established_options_rm_addr(struct sock *sk,
> >> int i, len;
> >>
> >> if (!mptcp_pm_should_rm_signal(msk) ||
> >> - !(mptcp_pm_rm_addr_signal(msk, remaining, &rm_list)))
> >> - return false;
> >> -
> >> - len = mptcp_rm_addr_len(&rm_list);
> >> - if (len < 0)
> >> - return false;
> >> - if (remaining < len)
> >> + !(mptcp_pm_rm_addr_signal(msk, remaining, &rm_list, &len)))
> >> return false;
> >>
> >> *size = len;
> >> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> >> index 9c5b15c..2311ea5 100644
> >> --- a/net/mptcp/pm.c
> >> +++ b/net/mptcp/pm.c
> >> @@ -255,9 +255,9 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
> >>
> >> bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> >> unsigned int opt_size, unsigned int remaining,
> >> - struct mptcp_out_options *opts, u8 *add_addr)
> >> + struct mptcp_out_options *opts, u8 *add_addr, int *len)
> >> {
> >> - int ret = false, len;
> >> + int ret = false;
> >>
> >> spin_lock_bh(&msk->pm.lock);
> >>
> >> @@ -276,8 +276,8 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> >> remaining += opt_size;
> >> }
> >>
> >> - len = mptcp_add_addr_len(msk, opts);
> >> - if (remaining < len)
> >> + *len = mptcp_add_addr_len(msk, opts);
> >> + if (remaining < *len)
> >> goto out_unlock;
> >>
> >> if ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)))
> >> @@ -287,17 +287,18 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> >>
> >> ret = true;
> >> out_unlock:
> >> + spin_unlock_bh(&msk->pm.lock);
> >> +
> >> if ((mptcp_pm_should_add_signal_echo(msk)) && (mptcp_pm_should_add_signal_addr(msk)))
> >> mptcp_pm_schedule_work(msk, MPTCP_PM_ADD_ADDR_SEND_ACK);
> >>
> >> - spin_unlock_bh(&msk->pm.lock);
> >> return ret;
> >> }
> >>
> >> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> - struct mptcp_rm_list *rm_list)
> >> + struct mptcp_rm_list *rm_list, int *len)
> >> {
> >> - int ret = false, len;
> >> + int ret = false;
> >> u8 rm_addr;
> >>
> >> spin_lock_bh(&msk->pm.lock);
> >> @@ -307,12 +308,12 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> goto out_unlock;
> >>
> >> rm_addr = msk->pm.addr_signal & ~BIT(MPTCP_RM_ADDR_SIGNAL);
> >> - len = mptcp_rm_addr_len(&msk->pm.rm_list_tx);
> >> - if (len < 0) {
> >> + *len = mptcp_rm_addr_len(&msk->pm.rm_list_tx);
> >> + if (*len < 0) {
> >> WRITE_ONCE(msk->pm.addr_signal, rm_addr);
> >> goto out_unlock;
> >> }
> >> - if (remaining < len)
> >> + if (remaining < *len)
> >> goto out_unlock;
> >>
> >> *rm_list = msk->pm.rm_list_tx;
> >> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> >> index caa4a60..5d7c9d7 100644
> >> --- a/net/mptcp/protocol.h
> >> +++ b/net/mptcp/protocol.h
> >> @@ -770,9 +770,9 @@ static inline int mptcp_rm_addr_len(const struct mptcp_rm_list *rm_list)
> >>
> >> bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> >> unsigned int opt_size, unsigned int remaining,
> >> - struct mptcp_out_options *opts, u8 *add_addr);
> >> + struct mptcp_out_options *opts, u8 *add_addr, int *len);
> >> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> >> - struct mptcp_rm_list *rm_list);
> >> + struct mptcp_rm_list *rm_list, int *len);
> >> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> >>
> >> void __init mptcp_pm_nl_init(void);
> >> --
> >> 1.8.3.1
> >>
> >>
> >
>
> --
> Li YongLong
next prev parent reply other threads:[~2021-07-02 8:06 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-30 10:24 [PATCH v7 0/4] mptcp: fix conflicts when using pm.add_signal in ADD_ADDR/echo and RM_ADDR process Yonglong Li
2021-06-30 10:24 ` [PATCH v7 1/5] mptcp: fix ADD_ADDR and RM_ADDR maybe flush addr_signal each other Yonglong Li
2021-06-30 10:37 ` Geliang Tang
2021-07-02 6:23 ` Yonglong Li
2021-06-30 10:24 ` [PATCH v7 2/5] mptcp: make MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO separate Yonglong Li
2021-06-30 10:24 ` [PATCH v7 3/5] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal Yonglong Li
2021-06-30 11:14 ` Geliang Tang
2021-07-02 6:15 ` Yonglong Li
2021-06-30 10:24 ` [PATCH v7 4/5] mptcp: remove some double-check Yonglong Li
2021-06-30 10:57 ` Geliang Tang
2021-07-02 6:22 ` Yonglong Li
2021-07-02 8:06 ` Geliang Tang [this message]
2021-06-30 10:24 ` [PATCH v7 5/5] mptcp: remove MPTCP_ADD_ADDR_IPV6 and MPTCP_ADD_ADDR_PORT Yonglong Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+WQbwvF_3FMVWjFD4=iOA87Quh6rGZpJ8htMYsexBCcGtNiAQ@mail.gmail.com' \
--to=geliangtang@gmail.com \
--cc=liyonglong@chinatelecom.cn \
--cc=mathew.j.martineau@linux.intel.com \
--cc=mptcp@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).