Re: [PATCH mptcp-net 2/2] selftests: mptcp: lib: get counters for the right test

On Wed, 2026-08-19 at 11:02 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 19/08/2026 10:34, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Thu, 2026-08-13 at 12:29 +0200, Matthieu Baerts (NGI0) wrote:
> > > When the value for a MIB counter is required,
> > > mptcp_lib_get_counter
> > > is
> > > called. It tries to use the cache, if available. If not it falls
> > > back
> > > to
> > > calling 'nstat' directly by looking at the absolute counters.
> > > 
> > > That's an issue for tests that don't recreate the netns for each
> > > subtest. In this case, 'nstat -a' will look at the counters for
> > > the
> > > netns.
> > > 
> > > Instead, it should look at the increment for the current test, by
> > > using
> > > the history recorded in /tmp/<ns>.nstat, if available, and not
> > > using
> > > '-a' which was dumping the absolute values.
> > > 
> > > While at it, rename the previous 'hist' variable to 'cache' as it
> > > was
> > > used to look at the cache, not the nstat history.
> > > 
> > > Fixes: 71388a9f331d ("selftests: mptcp: lib: get counters from
> > > nstat
> > > history")
> > > Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_lib.sh | 16 +++++++++---
> > > ----
> > >  1 file changed, 9 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > > b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > > index da1da414c30f..b9d14647f401 100644
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> > > @@ -416,19 +416,21 @@ mptcp_lib_nstat_get() {
> > >  }
> > >  
> > >  # $1: ns, $2: MIB counter
> > > -# Get the counter from the history
> > > (mptcp_lib_nstat_{init,get}()) if
> > > available.
> > > -# If not, get the counter from nstat ignoring any history.
> > > +# Get the counter from the cache (mptcp_lib_nstat_{init,get}())
> > > if
> > > available.
> > > +# If not, get the counter from nstat ignoring any cache, but
> > > using
> > > the history.
> > >  mptcp_lib_get_counter() {
> > >  	local ns="${1}"
> > >  	local counter="${2}"
> > > -	local hist="/tmp/${ns}.out"
> > > +	local cache="/tmp/${ns}.out"
> > > +	local hist="/tmp/${ns}.nstat"
> > >  	local count
> > >  
> > > -	if [[ -s "${hist}" && "${counter}" == *"Tcp"* ]]; then
> > > -		count=$(awk "/^${counter} / {print \$2; exit}"
> > > "${hist}")
> > > +	if [[ -s "${cache}" && "${counter}" == *"Tcp"* ]]; then
> > > +		count=$(awk "/^${counter} / {print \$2; exit}"
> > > "${cache}")
> > >  	else
> > > -		count=$(ip netns exec "${ns}" nstat -asz
> > > "${counter}" |
> > > -			awk 'NR==1 {next} {print $2}')
> > > +		count=$(NSTAT_HISTORY="${hist}" ip netns exec
> > > "${ns}" \
> > > +			nstat -sz "${counter}" |
> > > +				awk 'NR==1 {next} {print $2}')
> > >  	fi
> > >  	if [ -z "${count}" ]; then
> > >  		mptcp_lib_fail_if_expected_feature "${counter}
> > > counter"
> > 
> > I noticed that the code being modified in mptcp_lib_pr_nstat and
> > mptcp_lib_get_counter is duplicated. I'm wondering if we could
> > remove
> > this redundancy - for example, by creating a new helper for this
> > logic.
> 
> Thank you for the review and suggestion.
> 
> > # $1: ns ; $@: nstat arguments
> > # If cache exists, return it; otherwise run nstat.
> > mptcp_lib_nstat_cmd() {
> >     local ns="${1}"
> >     local cache="/tmp/${ns}.out"
> >     local hist="/tmp/${ns}.nstat"
> > 
> >     if [ -s "${cache}" ]; then
> >         cat "${cache}"
> >     else
> >         NSTAT_HISTORY="${hist}" ip netns exec "${ns}" nstat -sz
> > "${@}"
> >     fi
> > }
> > 
> > Then mptcp_lib_pr_nstat can be simplified to:
> > 
> > mptcp_lib_pr_nstat() {
> >     local ns="${1}"
> > 
> >     mptcp_lib_nstat_cmd "${ns}" |
> >             awk '/Tcp/ { print "  "$0 }'
> > }
> > 
> > And mptcp_lib_get_counter can be simplified to:
> > 
> > mptcp_lib_get_counter() {
> >     local ns="${1}"
> >     local counter="${2}"
> >     local count
> > 
> >     count=$(mptcp_lib_nstat_cmd "${ns}" "${counter}" |
> >             awk -v c="${counter}" '$1 == c {print $2; exit}')
> 
> The cache cannot be used to check non-Tcp counters because it only
> contains *Tcp* counters. Then that makes this code adding extra
> conditions, and I'm not sure the new helper is worth it. WDYT?
> 
> Maybe it could be added if other modifications are needed around
> these
> helpers, but here, I wouldn't do that in a series targeting -net.

Sure. I can also send a follow-up patch later. I have no other comments
on this series - I'll reply to the cover letter with my Reviewed-by tag
shortly.

> 
> Cheers,
> Matt