Re: [PATCH net-next 2/2] samples: pktgen: new append mode

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Fri, 22 Jan 2021 16:05:17 +0100
Igor Russkikh <irusskikh@marvell.com> wrote:

> To configure various complex flows we for sure can create custom
> pktgen init scripts, but sometimes thats not that easy.
> 
> New "-a" (append) option in all the existing sample scripts allows
> to append more "devices" into pktgen threads.
> 
> The most straightforward usecases for that are:
> - using multiple devices. We have to generate full linerate on
> all physical functions (ports) of our multiport device.
> - pushing multiple flows (with different packet options)

The use-case makes sense.

More comment inlined below.

> Signed-off-by: Igor Russkikh <irusskikh@marvell.com>
> ---
>  samples/pktgen/README.rst                      | 18 ++++++++++++++++++
>  samples/pktgen/functions.sh                    |  2 +-
>  samples/pktgen/parameters.sh                   |  7 ++++++-
>  samples/pktgen/pktgen_sample01_simple.sh       | 10 ++++++++--
>  samples/pktgen/pktgen_sample02_multiqueue.sh   | 10 ++++++++--
>  .../pktgen_sample03_burst_single_flow.sh       | 10 ++++++++--
>  samples/pktgen/pktgen_sample04_many_flows.sh   | 10 ++++++++--
>  .../pktgen/pktgen_sample05_flow_per_thread.sh  | 10 ++++++++--
>  ..._sample06_numa_awared_queue_irq_affinity.sh | 10 ++++++++--
>  9 files changed, 73 insertions(+), 14 deletions(-)
> 
> diff --git a/samples/pktgen/README.rst b/samples/pktgen/README.rst
> index f9c53ca5cf93..f7d8dd76b0c4 100644
> --- a/samples/pktgen/README.rst
> +++ b/samples/pktgen/README.rst
> @@ -28,10 +28,28 @@ across the sample scripts.  Usage example is printed on errors::
>    -b : ($BURST)     HW level bursting of SKBs
>    -v : ($VERBOSE)   verbose
>    -x : ($DEBUG)     debug
> +  -6 : ($IP6)       IPv6
> +  -w : ($DELAY)     Tx Delay value (us)
> +  -a : ($APPENDCFG) Script will not reset generator's state, but will append its config

You called it $APPENDCFG, but code use $APPEND.

>  The global variable being set is also listed.  E.g. the required
>  interface/device parameter "-i" sets variable $DEV.
>  
> +"-a" parameter may be used to create different flows simultaneously.
> +In this mode script will keep the existing config, will append its settings.
> +In this mode you'll have to manually run traffic with "pg_ctrl start".
> +
> +For example you may use:
> +
> +    source ./samples/pktgen/functions.sh
> +    pg_ctrl reset
> +    # add first device
> +    ./pktgen_sample06_numa_awared_queue_irq_affinity.sh -a -i ens1f0 -m 34:80:0d:a3:fc:c9 -t 8
> +    # add second device
> +    ./pktgen_sample06_numa_awared_queue_irq_affinity.sh -a -i ens1f1 -m 34:80:0d:a3:fc:c9 -t 8
> +    # run joint traffic on two devs
> +    pg_ctrl start
> +
>  Common functions
>  ----------------
>  The functions.sh file provides; Three different shell functions for
> diff --git a/samples/pktgen/functions.sh b/samples/pktgen/functions.sh
> index dae06d5b38fa..8db945ee4f55 100644
> --- a/samples/pktgen/functions.sh
> +++ b/samples/pktgen/functions.sh
> @@ -108,7 +108,7 @@ function pgset() {
>      fi
>  }
>  
> -[[ $EUID -eq 0 ]] && trap 'pg_ctrl "reset"' EXIT
> +[ -z "$APPEND" ] && [ "$EUID" -eq 0 ] && trap '[ -z "$APPEND" ] && pg_ctrl "reset"' EXIT

This looks confusing and wrong (I think).
(e.g. is the second '[ -z "$APPEND" ] && ...' needed).

In functions.sh we don't need to "compress" the lines that much. I
prefer readability in this file.  (Cc Daniel T. Lee as he added this
line).  Maybe we can make it more human readable:

if [[ -z "$APPEND" ]]; then
	if [[ $EUID -eq 0 ]]; then
		# Cleanup pktgen setup on exit
		trap 'pg_ctrl "reset"' EXIT
	fi
fi

I'm a little confused how the "trap" got added into 'functions.sh', as
my original intend was that function.sh should only provide helper
functions and not have a side-effect. (But I can see I acked the
change).

  
>  ## -- General shell tricks --
>  
> diff --git a/samples/pktgen/parameters.sh b/samples/pktgen/parameters.sh
> index 70cc2878d479..3fd4d5e8107a 100644
> --- a/samples/pktgen/parameters.sh
> +++ b/samples/pktgen/parameters.sh
> @@ -20,12 +20,13 @@ function usage() {
>      echo "  -x : (\$DEBUG)     debug"
>      echo "  -6 : (\$IP6)       IPv6"
>      echo "  -w : (\$DELAY)     Tx Delay value (us)"
> +    echo "  -a : (\$APPENDCFG) Script will not reset generator's state, but will append its config"

You called it $APPENDCFG, but code use $APPEND.

>      echo ""
>  }
>  
>  ##  --- Parse command line arguments / parameters ---
>  ## echo "Commandline options:"
> -while getopts "s:i:d:m:p:f:t:c:n:b:w:vxh6" option; do
> +while getopts "s:i:d:m:p:f:t:c:n:b:w:vxh6a" option; do
>      case $option in
>          i) # interface
>            export DEV=$OPTARG
> @@ -83,6 +84,10 @@ while getopts "s:i:d:m:p:f:t:c:n:b:w:vxh6" option; do
>  	  export IP6=6
>  	  info "IP6: IP6=$IP6"
>  	  ;;
> +        a)
> +          export APPEND=yes

See variable name $APPEND is used here, but help says $APPENDCFG

> +          info "Append mode: APPEND=$APPEND"
> +          ;;
>          h|?|*)
>            usage;
>            err 2 "[ERROR] Unknown parameters!!!"
> diff --git a/samples/pktgen/pktgen_sample01_simple.sh b/samples/pktgen/pktgen_sample01_simple.sh
> index c2ad1fa32d3f..8ca7913eaf8a 100755
> --- a/samples/pktgen/pktgen_sample01_simple.sh
> +++ b/samples/pktgen/pktgen_sample01_simple.sh
> @@ -37,11 +37,11 @@ UDP_SRC_MAX=109
>  
>  # General cleanup everything since last run
>  # (especially important if other threads were configured by other scripts)
> -pg_ctrl "reset"
> +[ -z "$APPEND" ] && pg_ctrl "reset"

Makes sense.

>  # Add remove all other devices and add_device $DEV to thread 0
>  thread=0
> -pg_thread $thread "rem_device_all"
> +[ -z "$APPEND" ] && pg_thread $thread "rem_device_all"
>  pg_thread $thread "add_device" $DEV
>  
>  # How many packets to send (zero means indefinitely)
> @@ -77,6 +77,8 @@ pg_set $DEV "flag UDPSRC_RND"
>  pg_set $DEV "udp_src_min $UDP_SRC_MIN"
>  pg_set $DEV "udp_src_max $UDP_SRC_MAX"
>  
> +if [ -z "$APPEND" ]; then
> +
>  # start_run
>  echo "Running... ctrl^C to stop" >&2
>  pg_ctrl "start"
> @@ -85,3 +87,7 @@ echo "Done" >&2
>  # Print results
>  echo "Result device: $DEV"
>  cat /proc/net/pktgen/$DEV
> +
> +else
> +echo "Append mode: config done. Do more or use 'pg_ctrl start' to run"
> +fi

Hmm, could we indent lines for readability?
(Same in other files)

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer