[v2] mptcp_join userspace pm tests

[PATCH mptcp-next v2 1/5] mptcp: update MIB_RMSUBFLOW in cmd_sf_destroy

Posted by Geliang Tang 3 years, 3 months ago

This patch increases MPTCP_MIB_RMSUBFLOW mib counter in userspace pm
destroy subflow function mptcp_nl_cmd_sf_destroy() when removing subflow.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index f56378e4f597..ebbab5200290 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -5,6 +5,7 @@
  */
 
 #include "protocol.h"
+#include "mib.h"
 
 void mptcp_free_local_addr_list(struct mptcp_sock *msk)
 {
@@ -418,6 +419,7 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 
 		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
 		mptcp_close_ssk(sk, ssk, subflow);
+		__MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
 		err = 0;
 	} else {
 		err = -ESRCH;
-- 
2.35.3

Re: [PATCH mptcp-next v2 1/5] mptcp: update MIB_RMSUBFLOW in cmd_sf_destroy

Posted by Matthieu Baerts 3 years, 3 months ago

Hi Geliang,

On 11/06/2022 16:54, Geliang Tang wrote:
> This patch increases MPTCP_MIB_RMSUBFLOW mib counter in userspace pm
> destroy subflow function mptcp_nl_cmd_sf_destroy() when removing subflow.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_userspace.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index f56378e4f597..ebbab5200290 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include "protocol.h"
> +#include "mib.h"
>  
>  void mptcp_free_local_addr_list(struct mptcp_sock *msk)
>  {
> @@ -418,6 +419,7 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
>  
>  		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
>  		mptcp_close_ssk(sk, ssk, subflow);
> +		__MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);

It looks like this instruction is causing quite a bit of issues
according to the public CI:

- KVM Validation: normal:
  - Critical: 7 Call Trace(s) ❌:
  - Task: https://cirrus-ci.com/task/5443636765655040
  - Summary:
https://api.cirrus-ci.com/v1/artifact/task/5443636765655040/summary/summary.txt


I guess you should use MPTCP_INC_STATS() instead.


Side note: I don't know if it is a good idea to increment MIB counters
from the PM code (any pm*.c files). I think this should only be done
from "core" functions in protocol.c and subflow.c (+ options.c of
course). Or from new helpers declared in protocol.h. WDYT?

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

Re: [PATCH mptcp-next v2 1/5] mptcp: update MIB_RMSUBFLOW in cmd_sf_destroy

Posted by Mat Martineau 3 years, 3 months ago

On Mon, 13 Jun 2022, Matthieu Baerts wrote:

> Hi Geliang,
>
> On 11/06/2022 16:54, Geliang Tang wrote:
>> This patch increases MPTCP_MIB_RMSUBFLOW mib counter in userspace pm
>> destroy subflow function mptcp_nl_cmd_sf_destroy() when removing subflow.
>>
>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>> ---
>>  net/mptcp/pm_userspace.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>> index f56378e4f597..ebbab5200290 100644
>> --- a/net/mptcp/pm_userspace.c
>> +++ b/net/mptcp/pm_userspace.c
>> @@ -5,6 +5,7 @@
>>   */
>>
>>  #include "protocol.h"
>> +#include "mib.h"
>>
>>  void mptcp_free_local_addr_list(struct mptcp_sock *msk)
>>  {
>> @@ -418,6 +419,7 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
>>
>>  		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
>>  		mptcp_close_ssk(sk, ssk, subflow);
>> +		__MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
>
> It looks like this instruction is causing quite a bit of issues
> according to the public CI:
>
> - KVM Validation: normal:
>  - Critical: 7 Call Trace(s) ❌:
>  - Task: https://cirrus-ci.com/task/5443636765655040
>  - Summary:
> https://api.cirrus-ci.com/v1/artifact/task/5443636765655040/summary/summary.txt
>
>
> I guess you should use MPTCP_INC_STATS() instead.

+1

>
>
> Side note: I don't know if it is a good idea to increment MIB counters
> from the PM code (any pm*.c files). I think this should only be done
> from "core" functions in protocol.c and subflow.c (+ options.c of
> course). Or from new helpers declared in protocol.h. WDYT?
>

Could you elaborate some more on this? There's some existing MIB counter 
code in the pm*.c files that doesn't seem too out of place - is the idea 
that moving calls to the core it would reduce duplicated code in 
pm_netlink.c and pm_userspace.c?


--
Mat Martineau
Intel

Re: [PATCH mptcp-next v2 1/5] mptcp: update MIB_RMSUBFLOW in cmd_sf_destroy

Posted by Matthieu Baerts 3 years, 3 months ago

Hi Mat,

On 14/06/2022 01:04, Mat Martineau wrote:
> On Mon, 13 Jun 2022, Matthieu Baerts wrote:
> 
>> Hi Geliang,
>>
>> On 11/06/2022 16:54, Geliang Tang wrote:
>>> This patch increases MPTCP_MIB_RMSUBFLOW mib counter in userspace pm
>>> destroy subflow function mptcp_nl_cmd_sf_destroy() when removing
>>> subflow.
>>>
>>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>>> ---
>>>  net/mptcp/pm_userspace.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
>>> index f56378e4f597..ebbab5200290 100644
>>> --- a/net/mptcp/pm_userspace.c
>>> +++ b/net/mptcp/pm_userspace.c
>>> @@ -5,6 +5,7 @@
>>>   */
>>>
>>>  #include "protocol.h"
>>> +#include "mib.h"
>>>
>>>  void mptcp_free_local_addr_list(struct mptcp_sock *msk)
>>>  {
>>> @@ -418,6 +419,7 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb,
>>> struct genl_info *info)
>>>
>>>          mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
>>>          mptcp_close_ssk(sk, ssk, subflow);
>>> +        __MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
>>
>> It looks like this instruction is causing quite a bit of issues
>> according to the public CI:
>>
>> - KVM Validation: normal:
>>  - Critical: 7 Call Trace(s) ❌:
>>  - Task: https://cirrus-ci.com/task/5443636765655040
>>  - Summary:
>> https://api.cirrus-ci.com/v1/artifact/task/5443636765655040/summary/summary.txt
>>
>>
>>
>> I guess you should use MPTCP_INC_STATS() instead.
> 
> +1
> 
>>
>>
>> Side note: I don't know if it is a good idea to increment MIB counters
>> from the PM code (any pm*.c files). I think this should only be done
>> from "core" functions in protocol.c and subflow.c (+ options.c of
>> course). Or from new helpers declared in protocol.h. WDYT?
>>
> 
> Could you elaborate some more on this? There's some existing MIB counter
> code in the pm*.c files that doesn't seem too out of place - is the idea
> that moving calls to the core it would reduce duplicated code in
> pm_netlink.c and pm_userspace.c?

Yes indeed, this is linked and probably more code should be refactored.

My point is that if you need to do a few "independent" calls to
functions from the core API to do a certain type of action from
different sides, it seems likely to have this code being out of sync
after a bit of time or in new code. I'm not sure I'm explaining clearly
what I have in mind so here is a "silly" example to explain this simple
thing:

If you want to destroy a subflow you might want to do:

    msk = get_msk(ssk); // you need the msk later, normal to have this

    // these two functions are linked: here we need to use both to have
    // the desired behaviour. We might not want to close the subflow
    // after the shutdown so fine without helpers (except if it is more
    // common to do both)
    shutdown_ssk(msk, ssk)
    close_ssk(msk, ssk);

    // it is easy to forget counters
    inc_stats(msk, REMOVE_ADDRESS); // maybe too specific: add helper
                                    // to avoid dup code in pm*.c and
                                    // inc the counter only in 1 place?
    dec_stats(msk, SSK_COUNTERS);   // this could go in core API func


But OK, now when reading the code in pm_netlink.c, adding a helper would
not really help so it is probably easier to keep this stat being
increased here I suppose.

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