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

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

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

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