From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from chinatelecom.cn (prt-mail.chinatelecom.cn [42.123.76.227]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 25B9671 for ; Mon, 21 Jun 2021 07:50:04 +0000 (UTC) HMM_SOURCE_IP:172.18.0.218:44446.1099823238 HMM_ATTACHE_NUM:0000 HMM_SOURCE_TYPE:SMTP Received: from clientip-36.111.140.26?logid-570fd38b3d3949c481d5f2b3aa51dffe (unknown [172.18.0.218]) by chinatelecom.cn (HERMES) with SMTP id 9412E2800FD; Mon, 21 Jun 2021 15:49:56 +0800 (CST) X-189-SAVE-TO-SEND: liyonglong@chinatelecom.cn Received: from ([172.18.0.218]) by app0025 with ESMTP id 570fd38b3d3949c481d5f2b3aa51dffe for qitiepeng@chinatelecom.cn; Mon Jun 21 15:49:56 2021 X-Transaction-ID: 570fd38b3d3949c481d5f2b3aa51dffe X-filter-score: filter<0> X-Real-From: liyonglong@chinatelecom.cn X-Receive-IP: 172.18.0.218 X-MEDUSA-Status: 0 Sender: liyonglong@chinatelecom.cn Subject: Re: [PATCH v4 3/4] mptcp: build ADD_ADDR/echo-ADD_ADDR option according pm.add_signal To: Geliang Tang Cc: mptcp@lists.linux.dev, Mat Martineau , qitiepeng@chinatelecom.cn References: <1624004309-54480-1-git-send-email-liyonglong@chinatelecom.cn> <1624004309-54480-4-git-send-email-liyonglong@chinatelecom.cn> <85720e69-d6d4-4a9b-9f1c-0898a1cf5009@chinatelecom.cn> From: Yonglong Li Message-ID: Date: Mon, 21 Jun 2021 15:49:51 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 X-Mailing-List: mptcp@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 2021/6/21 15:39, Geliang Tang wrote: > Yonglong Li 于2021年6月21日周一 下午3:16写道： >> >> >> >> On 2021/6/21 14:42, Geliang Tang wrote: >>> Hi Yonglong, >>> >>> Yonglong Li 于2021年6月21日周一 上午11:52写道： >>>> >>>> >>>> On 2021/6/18 19:20, Geliang Tang wrote: >>>>> Hi Yonglong, >>>>> >>>>> Thanks for v4! >>>>> >>>>> Yonglong Li 于2021年6月18日周五 下午4:19写道： >>>>>> according MPTCP_ADD_ADDR_SIGNAL and MPTCP_ADD_ADDR_ECHO flag build >>>>>> ADD_ADDR/echo-ADD_ADDR option >>>>>> >>>>>> add a suboptions type OPTION_MPTCP_ADD_ECHO to mark as echo option >>>>>> >>>>>> Signed-off-by: Yonglong Li >>>>>> --- >>>>>> net/mptcp/options.c | 124 +++++++++++++++++++++++++++++++-------------------- >>>>>> net/mptcp/pm.c | 30 ++++--------- >>>>>> net/mptcp/protocol.h | 13 +++--- >>>>>> 3 files changed, 92 insertions(+), 75 deletions(-) >>>>>> >>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >>>>>> index 1aec016..43e3241 100644 >>>>>> --- a/net/mptcp/options.c >>>>>> +++ b/net/mptcp/options.c >>>>>> @@ -655,41 +655,64 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff * >>>>>> struct mptcp_sock *msk = mptcp_sk(subflow->conn); >>>>>> bool drop_other_suboptions = false; >>>>>> unsigned int opt_size = *size; >>>>>> - bool echo; >>>>>> - bool port; >>>>>> + struct mptcp_addr_info remote; >>>>>> + struct mptcp_addr_info local; >>>>>> + u8 add_addr, flags = 0xff; >>>>>> int len; >>>>>> >>>>>> - if ((mptcp_pm_should_add_signal_ipv6(msk) || >>>>>> - mptcp_pm_should_add_signal_port(msk) || >>>>>> - mptcp_pm_should_add_signal_echo(msk)) && >>>>>> - skb && skb_is_tcp_pure_ack(skb)) { >>>>>> - pr_debug("drop other suboptions"); >>>>>> - opts->suboptions = 0; >>>>>> - opts->ext_copy.use_ack = 0; >>>>>> - opts->ext_copy.use_map = 0; >>>>>> - remaining += opt_size; >>>>>> - drop_other_suboptions = true; >>>>>> - } >>>>>> - >>>>>> - if (!mptcp_pm_should_add_signal(msk) || >>>>>> - !(mptcp_pm_add_addr_signal(msk, remaining, &opts->addr, &echo, &port))) >>>>>> - return false; >>>>>> - >>>>>> - len = mptcp_add_addr_len(opts->addr.family, echo, port); >>>>>> - if (remaining < len) >>>>>> + if (!mptcp_pm_should_add_signal(msk)) >>>>>> return false; >>>>>> >>>>>> - *size = len; >>>>>> - if (drop_other_suboptions) >>>>>> - *size -= opt_size; >>>>>> - opts->suboptions |= OPTION_MPTCP_ADD_ADDR; >>>>>> - if (!echo) { >>>>>> + *size = 0; >>>>>> + mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr); >>>>>> + if (mptcp_pm_should_add_signal_echo(msk)) { >>>>>> + if (skb && skb_is_tcp_pure_ack(skb)) { >>>>> ''' >>>>>> + pr_debug("drop other suboptions"); >>>>>> + opts->suboptions = 0; >>>>>> + opts->ext_copy.use_ack = 0; >>>>>> + opts->ext_copy.use_map = 0; >>>>>> + remaining += opt_size; >>>>>> + drop_other_suboptions = true; >>>>> ''' >>>>> >>>>>> + } >>>>>> + len = mptcp_add_addr_len(remote.family, true, !!remote.port); >>>>>> + if (remaining < len) >>>>>> + return false; >>>>>> + remaining -= len; >>>>>> + *size += len; >>>>>> + opts->remote = remote; >>>>>> + flags = (u8)~BIT(MPTCP_ADD_ADDR_ECHO); >>>>>> + opts->suboptions |= OPTION_MPTCP_ADD_ECHO; >>>>>> + pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x", >>>>>> + opts->remote.id, ntohs(opts->remote.port), add_addr); >>>>>> + } else if (mptcp_pm_should_add_signal_addr(msk)) { >>>>>> + if ((local.family == AF_INET6 || local.port) && skb && >>>>>> + skb_is_tcp_pure_ack(skb)) { >>>>> ''' >>>>>> + pr_debug("drop other suboptions"); >>>>>> + opts->suboptions = 0; >>>>>> + opts->ext_copy.use_ack = 0; >>>>>> + opts->ext_copy.use_map = 0; >>>>>> + remaining += opt_size; >>>>>> + drop_other_suboptions = true; >>>>> ''' >>>>> >>>>> I think this "drop other suboptions" trunk here is still duplicated. Can >>>>> we just use one "drop other suboptions" trunk only? >>>>> >>>>> Thanks. >>>>> -Geliang >>>>> >>>> Hi Geliang, Thanks for you replay. >>>> >>>> The commit "07f8252fe0e3c2b6320eeff18bdc5b7fb8845cb3" Davide said "echo-ed ADD_ADDR >>>> carried over pure TCP ACKs, so there is no need to add a DSS element that would fit >>>> only ADD_ADDR with IPv4 address.Drop the DSS from echo-ed ADD_ADDR, regardless of the >>>> IP version." >>>> ADD_ADDR option can add with DSS if the addr is IPv4. So I think it is more clear >>>> to decide "drop other suboptions" in two trunk. >>> Could we change it like this: >>> >>> ''' >>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >>> index e77b5d532fb8..8b4cb0581a49 100644 >>> --- a/net/mptcp/options.c >>> +++ b/net/mptcp/options.c >>> @@ -673,15 +673,20 @@ static bool >>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff * >>> >>> *size = 0; >>> mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr); >>> + >>> + if ((mptcp_pm_should_add_signal_echo(msk) || >>> + (mptcp_pm_should_add_signal_addr(msk) && >>> + (local.family == AF_INET6 || local.port))) && >>> + skb && skb_is_tcp_pure_ack(skb)) { >>> + pr_debug("drop other suboptions"); >>> + opts->suboptions = 0; >>> + opts->ext_copy.use_ack = 0; >>> + opts->ext_copy.use_map = 0; >>> + remaining += opt_size; >>> + drop_other_suboptions = true; >>> + } >>> + >>> if (mptcp_pm_should_add_signal_echo(msk)) { >>> - if (skb && skb_is_tcp_pure_ack(skb)) { >>> - pr_debug("drop other suboptions"); >>> - opts->suboptions = 0; >>> - opts->ext_copy.use_ack = 0; >>> - opts->ext_copy.use_map = 0; >>> - remaining += opt_size; >>> - drop_other_suboptions = true; >>> - } >>> len = mptcp_add_addr_len(remote.family, true, !!remote.port); >>> if (remaining < len) >>> return false; >>> @@ -693,15 +698,6 @@ static bool >>> mptcp_established_options_add_addr(struct sock *sk, struct sk_buff * >>> pr_debug("addr_id=%d, echo=1, port=%d addr_signal:%x", >>> opts->remote.id, ntohs(opts->remote.port), add_addr); >>> } else if (mptcp_pm_should_add_signal_addr(msk)) { >>> - if ((local.family == AF_INET6 || local.port) && skb && >>> - skb_is_tcp_pure_ack(skb)) { >>> - pr_debug("drop other suboptions"); >>> - opts->suboptions = 0; >>> - opts->ext_copy.use_ack = 0; >>> - opts->ext_copy.use_map = 0; >>> - remaining += opt_size; >>> - drop_other_suboptions = true; >>> - } >>> len = mptcp_add_addr_len(local.family, false, !!local.port); >>> if (remaining < len) >>> return false; >>> ''' >>> WDYT? >> Thanks for your advice. >> >> Because MPTCP_ADD_ADDR_ECHO and MPTCP_ADD_ADDR_SIGNAL can be set at the same time. So as your advice we should >> change like this(still I think it not clear than before): >> >> mptcp_pm_add_addr_signal(msk, &local, &remote, &add_addr); >> + if ((mptcp_pm_should_add_signal_echo(msk) || >> + (!mptcp_pm_should_add_signal_echo(msk) && >> + mptcp_pm_should_add_signal_addr(msk) && >> + (local.family == AF_INET6 || local.port))) && >> + skb && skb_is_tcp_pure_ack(skb)) { >> + pr_debug("drop other suboptions"); >> + opts->suboptions = 0; >> + opts->ext_copy.use_ack = 0; >> + opts->ext_copy.use_map = 0; >> + remaining += opt_size; >> + drop_other_suboptions = true; >> + } >> + >> if (mptcp_pm_should_add_signal_echo(msk)) { >> - if (skb && skb_is_tcp_pure_ack(skb)) { >> >> >>> >>>>> >>>>>> + } >>>>>> + len = mptcp_add_addr_len(local.family, false, !!local.port); >>>>>> + if (remaining < len) >>>>>> + return false; >>> And here, I think "remaining -= len;" is missing. >>> >>> Thanks, >>> -Geliang >>> >> "remaining" is not being used in the flowing code. So "remaining -=len;" is not necessary. But you remindme that the "remaining -= len;" can be removed in the first trunk. > > I think we should keep this 'remaining -= len;', remaining can be used > in tcp_established_options. > Thanks for your review. I think "remaining" will not use in tcp_established_options. "size" is used by tcp_established_options. >> >> I will send v5 as your advice. >> >