Re: [Intel-wired-lan] [PATCH v7 net-next 11/15] testing: net-drv: add basic shaper test

[Paolo Abeni <pabeni@redhat.com>]

On 9/10/24 23:41, Stanislav Fomichev wrote:
> On 09/10, Paolo Abeni wrote:
>> Leverage a basic/dummy netdevsim implementation to do functional
>> coverage for NL interface.
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>> v5 -> v6:
>>    - additional test-cases for delegation and queue reconf
>>
>> v4 -> v5:
>>    - updated to new driver API
>>    - more consistent indentation
>>
>> rfc v1 -> v2:
>>    - added more test-cases WRT nesting and grouping
>> ---
>>   drivers/net/Kconfig                           |   1 +
>>   drivers/net/netdevsim/ethtool.c               |   2 +
>>   drivers/net/netdevsim/netdev.c                |  39 ++
>>   tools/testing/selftests/drivers/net/Makefile  |   1 +
>>   tools/testing/selftests/drivers/net/shaper.py | 457 ++++++++++++++++++
>>   .../testing/selftests/net/lib/py/__init__.py  |   1 +
>>   tools/testing/selftests/net/lib/py/ynl.py     |   5 +
>>   7 files changed, 506 insertions(+)
>>   create mode 100755 tools/testing/selftests/drivers/net/shaper.py
>>
>> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>> index 9920b3a68ed1..1fd5acdc73c6 100644
>> --- a/drivers/net/Kconfig
>> +++ b/drivers/net/Kconfig
>> @@ -641,6 +641,7 @@ config NETDEVSIM
>>   	depends on PTP_1588_CLOCK_MOCK || PTP_1588_CLOCK_MOCK=n
>>   	select NET_DEVLINK
>>   	select PAGE_POOL
>> +	select NET_SHAPER
>>   	help
>>   	  This driver is a developer testing tool and software model that can
>>   	  be used to test various control path networking APIs, especially
>> diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
>> index 1436905bc106..5fe1eaef99b5 100644
>> --- a/drivers/net/netdevsim/ethtool.c
>> +++ b/drivers/net/netdevsim/ethtool.c
>> @@ -103,8 +103,10 @@ nsim_set_channels(struct net_device *dev, struct ethtool_channels *ch)
>>   	struct netdevsim *ns = netdev_priv(dev);
>>   	int err;
>>   
>> +	mutex_lock(&dev->lock);
>>   	err = netif_set_real_num_queues(dev, ch->combined_count,
>>   					ch->combined_count);
>> +	mutex_unlock(&dev->lock);
>>   	if (err)
>>   		return err;
>>   
>> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>> index 017a6102be0a..cad85bb0cf54 100644
>> --- a/drivers/net/netdevsim/netdev.c
>> +++ b/drivers/net/netdevsim/netdev.c
>> @@ -22,6 +22,7 @@
>>   #include <net/netdev_queues.h>
>>   #include <net/page_pool/helpers.h>
>>   #include <net/netlink.h>
>> +#include <net/net_shaper.h>
>>   #include <net/pkt_cls.h>
>>   #include <net/rtnetlink.h>
>>   #include <net/udp_tunnel.h>
>> @@ -475,6 +476,43 @@ static int nsim_stop(struct net_device *dev)
>>   	return 0;
>>   }
>>   
>> +static int nsim_shaper_set(struct net_shaper_binding *binding,
>> +			   const struct net_shaper *shaper,
>> +			   struct netlink_ext_ack *extack)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int nsim_shaper_del(struct net_shaper_binding *binding,
>> +			   const struct net_shaper_handle *handle,
>> +			   struct netlink_ext_ack *extack)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int nsim_shaper_group(struct net_shaper_binding *binding,
>> +			     int leaves_count,
>> +			     const struct net_shaper *leaves,
>> +			     const struct net_shaper *root,
>> +			     struct netlink_ext_ack *extack)
>> +{
>> +	return 0;
>> +}
>> +
>> +static void nsim_shaper_cap(struct net_shaper_binding *binding,
>> +			    enum net_shaper_scope scope,
>> +			    unsigned long *flags)
>> +{
>> +	*flags = ULONG_MAX;
>> +}
>> +
>> +static const struct net_shaper_ops nsim_shaper_ops = {
>> +	.set			= nsim_shaper_set,
>> +	.delete			= nsim_shaper_del,
>> +	.group			= nsim_shaper_group,
>> +	.capabilities		= nsim_shaper_cap,
>> +};
>> +
>>   static const struct net_device_ops nsim_netdev_ops = {
>>   	.ndo_start_xmit		= nsim_start_xmit,
>>   	.ndo_set_rx_mode	= nsim_set_rx_mode,
>> @@ -496,6 +534,7 @@ static const struct net_device_ops nsim_netdev_ops = {
>>   	.ndo_bpf		= nsim_bpf,
>>   	.ndo_open		= nsim_open,
>>   	.ndo_stop		= nsim_stop,
>> +	.net_shaper_ops		= &nsim_shaper_ops,
>>   };
>>   
>>   static const struct net_device_ops nsim_vf_netdev_ops = {
>> diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile
>> index 39fb97a8c1df..25aec5c081df 100644
>> --- a/tools/testing/selftests/drivers/net/Makefile
>> +++ b/tools/testing/selftests/drivers/net/Makefile
>> @@ -9,6 +9,7 @@ TEST_PROGS := \
>>   	ping.py \
>>   	queues.py \
>>   	stats.py \
>> +	shaper.py
>>   # end of TEST_PROGS
>>   
>>   include ../../lib.mk
>> diff --git a/tools/testing/selftests/drivers/net/shaper.py b/tools/testing/selftests/drivers/net/shaper.py
>> new file mode 100755
>> index 000000000000..3504d51985bc
>> --- /dev/null
>> +++ b/tools/testing/selftests/drivers/net/shaper.py
>> @@ -0,0 +1,457 @@
>> +#!/usr/bin/env python3
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +from lib.py import ksft_run, ksft_exit, ksft_eq, ksft_true, KsftSkipEx
>> +from lib.py import EthtoolFamily, NetshaperFamily
>> +from lib.py import NetDrvEnv
>> +from lib.py import NlError
>> +from lib.py import cmd
>> +
>> +def get_shapers(cfg, nl_shaper) -> None:
>> +    try:
>> +        shapers = nl_shaper.get({'ifindex': cfg.ifindex}, dump=True)
>> +    except NlError as e:
>> +        if e.error == 95:
>> +            raise KsftSkipEx("shapers not supported by the device")
>> +        raise
>> +
>> +    # Default configuration: no shapers configured.
>> +    ksft_eq(len(shapers), 0)
>> +
>> +def get_caps(cfg, nl_shaper) -> None:
>> +    try:
>> +        caps = nl_shaper.cap_get({'ifindex': cfg.ifindex}, dump=True)
>> +    except NlError as e:
>> +        if e.error == 95:
>> +            raise KsftSkipEx("shapers not supported by the device")
>> +        raise
>> +
>> +    # Each device implementing shaper support must support some
>> +    # features in at least a scope.
>> +    ksft_true(len(caps)> 0)
>> +
>> +def set_qshapers(cfg, nl_shaper) -> None:
>> +    try:
>> +        caps = nl_shaper.cap_get({'ifindex': cfg.ifindex,
>> +                                 'scope':'queue'})
>> +    except NlError as e:
>> +        if e.error == 95:
>> +            raise KsftSkipEx("shapers not supported by the device")
>> +        raise
>> +    if not 'support-bw-max' in caps or not 'support-metric-bps' in caps:
>> +        raise KsftSkipEx("device does not support queue scope shapers with bw_max and metric bps")
>> +
>> +    cfg.queues = True;
>> +    netnl = EthtoolFamily()
>> +    channels = netnl.channels_get({'header': {'dev-index': cfg.ifindex}})
>> +    if channels['combined-count'] == 0:
>> +        cfg.rx_type = 'rx'
>> +        cfg.nr_queues = channels['rx-count']
>> +    else:
>> +        cfg.rx_type = 'combined'
>> +        cfg.nr_queues = channels['combined-count']
>> +    if cfg.nr_queues < 3:
>> +        raise KsftSkipEx("device does not support enough queues min 3 found {cfg.nr_queues}")
>> +
>> +    nl_shaper.set({'ifindex': cfg.ifindex,
>> +                   'handle': {'scope': 'queue', 'id': 1},
>> +                   'metric': 'bps',
>> +                   'bw-max': 10000})
>> +    nl_shaper.set({'ifindex': cfg.ifindex,
>> +                   'handle': {'scope': 'queue', 'id': 2},
>> +                   'metric': 'bps',
>> +                   'bw-max': 20000})
>> +
>> +    # Querying a specific shaper not yet configured must fail.
>> +    raised = False
>> +    try:
>> +        shaper_q0 = nl_shaper.get({'ifindex': cfg.ifindex,
>> +                                   'handle': {'scope': 'queue', 'id': 0}})
>> +    except (NlError):
>> +        raised = True
>> +    ksft_eq(raised, True)
>> +
>> +    shaper_q1 = nl_shaper.get({'ifindex': cfg.ifindex,
>> +                              'handle': {'scope': 'queue', 'id': 1}})
> 
> [..]
> 
>> +    ksft_eq(shaper_q1, {'ifindex': cfg.ifindex,
>> +                        'parent': {'scope': 'netdev'},
>> +                        'handle': {'scope': 'queue', 'id': 1},
>> +                        'metric': 'bps',
>> +                        'bw-max': 10000})
>> +
> 
> Before comparison, you probably need to drop some fields that are not
> expected?
> 
> # # Check failed {'ifindex': 8, 'parent': {'scope': 'netdev'}, 'handle': {'scope': 'queue', 'id': 1}, 'metric': 'bps', 'bw-min': 517778718638633216, 'bw-max': 10000, 'burst': 18446683600580769792, 'priority': 60858368, 'weight': 4294936704} != {'ifindex': 8, 'parent': {'scope': 'netdev'}, 'handle': {'scope': 'queue', 'id': 1}, 'metric': 'bps', 'bw-max': 10000}
> # # Check| At /home/virtme/testing-18/tools/testing/selftests/drivers/net/./shaper.py, line 83, in set_qshapers:
> # # Check|     ksft_eq(shapers, [{'ifindex': cfg.ifindex,
> # # Check failed [{'ifindex': 8, 'parent': {'scope': 'netdev'}, 'handle': {'scope': 'queue', 'id': 1}, 'metric': 'bps', 'bw-min': 517778718638633216, 'bw-max': 10000, 'burst': 18446683600580769792, 'priority': 60858368, 'weight': 4294936704}, {'ifindex': 8, 'parent': {'scope': 'netdev'}, 'handle': {'scope': 'queue', 'id': 2}, 'metric': 'bps', 'bw-min': 517778718638633216, 'bw-max': 20000, 'burst': 18446683600580769792, 'priority': 60858368, 'weight': 4294936704}] != [{'ifindex': 8, 'parent': {'scope': 'netdev'}, 'handle': {'scope': 'queue', 'id': 1}, 'metric': 'bps', 'bw-max': 10000}, {'ifindex': 8, 'parent': {'scope': 'netdev'}, 'handle': {'scope': 'queue', 'id': 2}, 'metric': 'bps', 'bw-max': 20000}]
> 
> https://netdev-3.bots.linux.dev/vmksft-net-drv-dbg/results/766702/4-shaper-py/stdout
> 
> Debug builds are also reporting a UBSAN error:
> 
> https://netdev-3.bots.linux.dev/vmksft-net-drv-dbg/results/766702/4-shaper-py/stderr

Thanks for the feedback, and sorry for the late reply, I was traveling.

It looks like the root cause is the same, a couple of stack allocated 
structs are not zeroed before usage.

For the record I could not reproduce the issue locally, because I 
probably miss some debug kconf.

I'll address the issue in the next iteration.

Thanks!

Paolo