Only the in-kernel PM uses the number of address and subflow limits
allowed per connection.

It then makes more sense not to display such info when other PMs are
used not to confuse the userspace by showing limits not being used.

While at it, we can get rid of the "val" variable and add indentations
instead.

Fixes: 3fd4c2a2d672 ("mptcp: bypass in-kernel PM restrictions for non-kernel PMs")
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/sockopt.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index f7842a0b6536..fa1e72235f28 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -889,7 +889,6 @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
 void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 {
 	u32 flags = 0;
-	u8 val;
 
 	memset(info, 0, sizeof(*info));
 
@@ -897,12 +896,19 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 	info->mptcpi_add_addr_signal = READ_ONCE(msk->pm.add_addr_signaled);
 	info->mptcpi_add_addr_accepted = READ_ONCE(msk->pm.add_addr_accepted);
 	info->mptcpi_local_addr_used = READ_ONCE(msk->pm.local_addr_used);
-	info->mptcpi_subflows_max = mptcp_pm_get_subflows_max(msk);
-	val = mptcp_pm_get_add_addr_signal_max(msk);
-	info->mptcpi_add_addr_signal_max = val;
-	val = mptcp_pm_get_add_addr_accept_max(msk);
-	info->mptcpi_add_addr_accepted_max = val;
-	info->mptcpi_local_addr_max = mptcp_pm_get_local_addr_max(msk);
+
+	/* The following limits only make sense for the in-kernel PM */
+	if (mptcp_pm_is_kernel(msk)) {
+		info->mptcpi_subflows_max =
+			mptcp_pm_get_subflows_max(msk);
+		info->mptcpi_add_addr_signal_max =
+			mptcp_pm_get_add_addr_signal_max(msk);
+		info->mptcpi_add_addr_accepted_max =
+			mptcp_pm_get_add_addr_accept_max(msk);
+		info->mptcpi_local_addr_max =
+			mptcp_pm_get_local_addr_max(msk);
+	}
+
 	if (test_bit(MPTCP_FALLBACK_DONE, &msk->flags))
 		flags |= MPTCP_INFO_FLAG_FALLBACK;
 	if (READ_ONCE(msk->can_ack))

---
base-commit: 7264bacd354401e60be232ea28eb4a8a249937e4
change-id: 20230223-mptcp-diag-info-limits-in-kernel-pm-65bf3b67132d

Best regards,
-- 
Matthieu Baerts <matthieu.baerts@tessares.net>

Hi Paolo,

On 23/02/2023 12:20, Matthieu Baerts wrote:
> Only the in-kernel PM uses the number of address and subflow limits
> allowed per connection.
> 
> It then makes more sense not to display such info when other PMs are
> used not to confuse the userspace by showing limits not being used.
> 
> While at it, we can get rid of the "val" variable and add indentations
> instead.
> 
> Fixes: 3fd4c2a2d672 ("mptcp: bypass in-kernel PM restrictions for non-kernel PMs")
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>

Thank you for the review (on IRC).

Now in our tree (feat. for next) without the Fixes tag but with your ACK:

New patches for t/upstream:
- 5435c97679c1: mptcp: do not fill info not used by the PM in used
- Results: 6ce53727880d..fc5dcb18c972 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230309T093419

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Hi Matt, Paolo,

Sorry for the late reply. I think these limits are still useful for
userspace PM too, although we haven't used them yet. Since we also do
not allow userspace PM to use addresses without limits.

If you also think it's necessary to use these limits, let's add a
ticket for this on Github. And I'll look at it.

If so, I think this patch should be reverted except the var 'val'
should be dropped indeed. How about only dropping 'val' in this patch?
WDYT?

Thanks,
-Geliang

Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年3月9日周四 17:35写道：
>
> Hi Paolo,
>
> On 23/02/2023 12:20, Matthieu Baerts wrote:
> > Only the in-kernel PM uses the number of address and subflow limits
> > allowed per connection.
> >
> > It then makes more sense not to display such info when other PMs are
> > used not to confuse the userspace by showing limits not being used.
> >
> > While at it, we can get rid of the "val" variable and add indentations
> > instead.
> >
> > Fixes: 3fd4c2a2d672 ("mptcp: bypass in-kernel PM restrictions for non-kernel PMs")
> > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>
> Thank you for the review (on IRC).
>
> Now in our tree (feat. for next) without the Fixes tag but with your ACK:
>
> New patches for t/upstream:
> - 5435c97679c1: mptcp: do not fill info not used by the PM in used
> - Results: 6ce53727880d..fc5dcb18c972 (export)
>
> Tests are now in progress:
>
> https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230309T093419
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>

Hi Geliang,

On 15/03/2023 06:09, Geliang Tang wrote:
> Sorry for the late reply. I think these limits are still useful for
> userspace PM too, although we haven't used them yet. Since we also do
> not allow userspace PM to use addresses without limits.

I think it is better to let the userspace PM daemon managing these
limits itself. I guess that's why the limits have been explicitly
bypassed for this PM, see commit 3fd4c2a2d672 ("mptcp: bypass in-kernel
PM restrictions for non-kernel PMs").

> If you also think it's necessary to use these limits, let's add a
> ticket for this on Github. And I'll look at it.
> 
> If so, I think this patch should be reverted except the var 'val'
> should be dropped indeed. How about only dropping 'val' in this patch?
> WDYT?

I think we should keep this patch and have these limits only used by the
in-kernel PM. Except if you see a case where these limits cannot be
properly handled by the userspace PM daemon?

Also, we might want to avoid the complexity of tracking in the kernel
which ADD_ADDR have been received and how they are being used by the
userspace, etc. But that's a technical detail.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net