[MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 1/2] mptcp: drop rm_addr_signal flag

[MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 1/2] mptcp: drop rm_addr_signal flag

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3664 bytes --]

On Thu, 19 Nov 2020, Geliang Tang wrote:

> This patch reused add_addr_signal for the RM_ADDR announce signal, by
> defining a new add_addr status named MPTCP_RM_ADDR_SIGNAL. Then the flag
> rm_addr_signal in PM could be dropped.
>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> net/mptcp/pm.c       | 22 +++++++++++++++++-----
> net/mptcp/protocol.h |  4 ++--
> 2 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index e63e14f4cf2a..cc424222894b 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -18,7 +18,12 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> {
> 	u8 add_addr = READ_ONCE(msk->pm.add_addr_signal);
>
> -	pr_debug("msk=%p, local_id=%d", msk, addr->id);
> +	pr_debug("msk=%p, local_id=%d, add_addr=%d", msk, addr->id, add_addr);
> +
> +	if (add_addr) {
> +		pr_err("addr_signal error.");

Hi Geliang,

One minor thing, pr_err() indicates a more serious problem that a user 
might need to do something about. These seem like pr_warn() at most. There 
is only one pr_err() in the rest of the MPTCP code.


Mat


> +		return -EINVAL;
> +	}
>
> 	msk->pm.local = *addr;
> 	add_addr |= BIT(MPTCP_ADD_ADDR_SIGNAL);
> @@ -34,10 +39,18 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>
> int mptcp_pm_remove_addr(struct mptcp_sock *msk, u8 local_id)
> {
> -	pr_debug("msk=%p, local_id=%d", msk, local_id);
> +	u8 rm_addr = READ_ONCE(msk->pm.add_addr_signal);
> +
> +	pr_debug("msk=%p, local_id=%d, rm_addr=%d", msk, local_id, rm_addr);
> +
> +	if (rm_addr) {
> +		pr_err("addr_signal error.");
> +		return -EINVAL;
> +	}
>
> 	msk->pm.rm_id = local_id;
> -	WRITE_ONCE(msk->pm.rm_addr_signal, true);
> +	rm_addr |= BIT(MPTCP_RM_ADDR_SIGNAL);
> +	WRITE_ONCE(msk->pm.add_addr_signal, rm_addr);
> 	return 0;
> }
>
> @@ -231,7 +244,7 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> 		goto out_unlock;
>
> 	*rm_id = msk->pm.rm_id;
> -	WRITE_ONCE(msk->pm.rm_addr_signal, false);
> +	WRITE_ONCE(msk->pm.add_addr_signal, 0);
> 	ret = true;
>
> out_unlock:
> @@ -253,7 +266,6 @@ void mptcp_pm_data_init(struct mptcp_sock *msk)
> 	msk->pm.rm_id = 0;
> 	WRITE_ONCE(msk->pm.work_pending, false);
> 	WRITE_ONCE(msk->pm.add_addr_signal, 0);
> -	WRITE_ONCE(msk->pm.rm_addr_signal, false);
> 	WRITE_ONCE(msk->pm.accept_addr, false);
> 	WRITE_ONCE(msk->pm.accept_subflow, false);
> 	msk->pm.status = 0;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index d926deae8610..05c853e337a0 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -172,6 +172,7 @@ enum mptcp_add_addr_status {
> 	MPTCP_ADD_ADDR_ECHO,
> 	MPTCP_ADD_ADDR_IPV6,
> 	MPTCP_ADD_ADDR_PORT,
> +	MPTCP_RM_ADDR_SIGNAL,
> };
>
> struct mptcp_pm_data {
> @@ -182,7 +183,6 @@ struct mptcp_pm_data {
> 	spinlock_t	lock;		/*protects the whole PM data */
>
> 	u8		add_addr_signal;
> -	bool		rm_addr_signal;
> 	bool		server_side;
> 	bool		work_pending;
> 	bool		accept_addr;
> @@ -564,7 +564,7 @@ static inline bool mptcp_pm_should_add_signal_port(struct mptcp_sock *msk)
>
> static inline bool mptcp_pm_should_rm_signal(struct mptcp_sock *msk)
> {
> -	return READ_ONCE(msk->pm.rm_addr_signal);
> +	return READ_ONCE(msk->pm.add_addr_signal) & BIT(MPTCP_RM_ADDR_SIGNAL);
> }
>
> static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> -- 
> 2.26.2
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org
>

--
Mat Martineau
Intel

[MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 1/2] mptcp: drop rm_addr_signal flag

[Geliang Tang]

[-- Attachment #1: Type: text/plain, Size: 2459 bytes --]

Hi Paolo,

Geliang Tang <geliangtang(a)gmail.com> 于2020年11月20日周五 下午5:39写道：
>
> Hi Paolo,
>
> Paolo Abeni <pabeni(a)redhat.com> 于2020年11月20日周五 下午5:17写道：
> >
> > On Thu, 2020-11-19 at 17:12 -0800, Mat Martineau wrote:
> > > On Thu, 19 Nov 2020, Geliang Tang wrote:
> > >
> > > > This patch reused add_addr_signal for the RM_ADDR announce signal, by
> > > > defining a new add_addr status named MPTCP_RM_ADDR_SIGNAL. Then the flag
> > > > rm_addr_signal in PM could be dropped.
> > > >
> > > > Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> > > > ---
> > > > net/mptcp/pm.c       | 22 +++++++++++++++++-----
> > > > net/mptcp/protocol.h |  4 ++--
> > > > 2 files changed, 19 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > > > index e63e14f4cf2a..cc424222894b 100644
> > > > --- a/net/mptcp/pm.c
> > > > +++ b/net/mptcp/pm.c
> > > > @@ -18,7 +18,12 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> > > > {
> > > >     u8 add_addr = READ_ONCE(msk->pm.add_addr_signal);
> > > >
> > > > -   pr_debug("msk=%p, local_id=%d", msk, addr->id);
> > > > +   pr_debug("msk=%p, local_id=%d, add_addr=%d", msk, addr->id, add_addr);
> > > > +
> > > > +   if (add_addr) {
> > > > +           pr_err("addr_signal error.");
> > >
> > > Hi Geliang,
> > >
> > > One minor thing, pr_err() indicates a more serious problem that a user
> > > might need to do something about. These seem like pr_warn() at most. There
> > > is only one pr_err() in the rest of the MPTCP code.
> >
> > Looks like the condition checked there could happen just due to some
> > bugs in the pm code, right?
> >
> > If so we could conside a WARN_ON_ONCE instead.
> >
>
> Yes, it happens just due to some bugs. I used WARN_ON_ONCE in v1, but
> WARN_ON_ONCE can trigger a kernel panic. I think we should avoid using it.
>
> > Otherwise, if is expected to enter that code path on some unusual
> > situation a pr_debug would be enough.
> >
> > You can also add the current add_addr value.

Yes, I agree with you, add the current add_addr value is much better. I'll
update it in v4.

Thanks.

-Geliang

>
> I didn't add the current add_addr value since it is printed out in the pr_debug
> a little bit ahead.
>
> I have sent out v3 of this patch, it use pr_warm instead of pr_err.
>
> Thanks.
>
> -Geliang
>
> >
> > /P
> >
> >
> >

[MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 1/2] mptcp: drop rm_addr_signal flag

[Geliang Tang]

[-- Attachment #1: Type: text/plain, Size: 2113 bytes --]

Hi Paolo,

Paolo Abeni <pabeni(a)redhat.com> 于2020年11月20日周五 下午5:17写道：
>
> On Thu, 2020-11-19 at 17:12 -0800, Mat Martineau wrote:
> > On Thu, 19 Nov 2020, Geliang Tang wrote:
> >
> > > This patch reused add_addr_signal for the RM_ADDR announce signal, by
> > > defining a new add_addr status named MPTCP_RM_ADDR_SIGNAL. Then the flag
> > > rm_addr_signal in PM could be dropped.
> > >
> > > Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> > > ---
> > > net/mptcp/pm.c       | 22 +++++++++++++++++-----
> > > net/mptcp/protocol.h |  4 ++--
> > > 2 files changed, 19 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > > index e63e14f4cf2a..cc424222894b 100644
> > > --- a/net/mptcp/pm.c
> > > +++ b/net/mptcp/pm.c
> > > @@ -18,7 +18,12 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> > > {
> > >     u8 add_addr = READ_ONCE(msk->pm.add_addr_signal);
> > >
> > > -   pr_debug("msk=%p, local_id=%d", msk, addr->id);
> > > +   pr_debug("msk=%p, local_id=%d, add_addr=%d", msk, addr->id, add_addr);
> > > +
> > > +   if (add_addr) {
> > > +           pr_err("addr_signal error.");
> >
> > Hi Geliang,
> >
> > One minor thing, pr_err() indicates a more serious problem that a user
> > might need to do something about. These seem like pr_warn() at most. There
> > is only one pr_err() in the rest of the MPTCP code.
>
> Looks like the condition checked there could happen just due to some
> bugs in the pm code, right?
>
> If so we could conside a WARN_ON_ONCE instead.
>

Yes, it happens just due to some bugs. I used WARN_ON_ONCE in v1, but
WARN_ON_ONCE can trigger a kernel panic. I think we should avoid using it.

> Otherwise, if is expected to enter that code path on some unusual
> situation a pr_debug would be enough.
>
> You can also add the current add_addr value.

I didn't add the current add_addr value since it is printed out in the pr_debug
a little bit ahead.

I have sent out v3 of this patch, it use pr_warm instead of pr_err.

Thanks.

-Geliang

>
> /P
>
>
>

[MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 1/2] mptcp: drop rm_addr_signal flag

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

On Thu, 2020-11-19 at 17:12 -0800, Mat Martineau wrote:
> On Thu, 19 Nov 2020, Geliang Tang wrote:
> 
> > This patch reused add_addr_signal for the RM_ADDR announce signal, by
> > defining a new add_addr status named MPTCP_RM_ADDR_SIGNAL. Then the flag
> > rm_addr_signal in PM could be dropped.
> > 
> > Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> > ---
> > net/mptcp/pm.c       | 22 +++++++++++++++++-----
> > net/mptcp/protocol.h |  4 ++--
> > 2 files changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index e63e14f4cf2a..cc424222894b 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -18,7 +18,12 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
> > {
> > 	u8 add_addr = READ_ONCE(msk->pm.add_addr_signal);
> > 
> > -	pr_debug("msk=%p, local_id=%d", msk, addr->id);
> > +	pr_debug("msk=%p, local_id=%d, add_addr=%d", msk, addr->id, add_addr);
> > +
> > +	if (add_addr) {
> > +		pr_err("addr_signal error.");
> 
> Hi Geliang,
> 
> One minor thing, pr_err() indicates a more serious problem that a user 
> might need to do something about. These seem like pr_warn() at most. There 
> is only one pr_err() in the rest of the MPTCP code.

Looks like the condition checked there could happen just due to some
bugs in the pm code, right?

If so we could conside a WARN_ON_ONCE instead.

Otherwise, if is expected to enter that code path on some unusual
situation a pr_debug would be enough.

You can also add the current add_addr value.

/P