Subject: Re: [net-next PATCH v2 2/2] net: samples: pktgen mode samples/tests for qdisc layer

From: John Fastabend <[email-masked]>
Date: Thu, 30 Jun 2016 09:39:57 -0700

On 16-06-30 01:23 AM, Jesper Dangaard Brouer wrote:
> On Wed, 29 Jun 2016 13:03:26 -0700
> John Fastabend <[email-masked]> wrote:
> 
>> This adds samples for pktgen to use with new mode to inject pkts into
>> the qdisc layer. This also doubles as nice test cases to test any
>> patches against qdisc layer.

[...]

>> +#
>> +# Benchmark script:
>> +#  - developed for benchmarking egress qdisc path, derived from
>> +#    ingress benchmark script.
>> +#

As you probably gathered 'derived' is giving me too much credit here
its more like cut'n'pasted from ingress benchmark scrip :)

>> +# Script for injecting packets into egress qdisc path of the stack
>> +# with pktgen "xmit_mode queue_xmit".
>> +#
>> +basedir=`dirname $0`
>> +source ${basedir}/functions.sh
>> +root_check_run_with_sudo "$@"
>> +
>> +# Parameter parsing via include
>> +source ${basedir}/parameters.sh
>> +# Using invalid DST_MAC will cause the packets to get dropped in
>> +# ip_rcv() which is part of the test
>> +[ -z "$DEST_IP" ] && DEST_IP="198.18.0.42"
>> +[ -z "$DST_MAC" ] && DST_MAC="90:e2:ba:ff:ff:ff"
>> +
>> +# Burst greater than 1 are invalid but allow users to specify it and
>> +# get an error instead of silently ignoring it.
>> +[ -z "$BURST" ] && BURST=1
> 
> In other scripts I've rejected this at this step, instead of depending
> on failure when sending the burst option to pktgen. Like:
> 
> https://github.com/netoptimizer/network-testing/blob/master/pktgen/pktgen_sample04_many_flows.sh#L31-L33
> 

Agreed that is nicer. I had originally left it to make sure I was
catching the burst > 1 case in pktgen but will remove.

>> +
>> +# Base Config
>> +DELAY="0"        # Zero means max speed
>> +COUNT="10000000" # Zero means indefinitely
>> +
>> +# General cleanup everything since last run
>> +pg_ctrl "reset"
>> +
>> +# Threads are specified with parameter -t value in $THREADS
>> +for ((thread = 0; thread < $THREADS; thread++)); do
>> +    # The device name is extended with @name, using thread number to
>> +    # make then unique, but any name will do.
>> +    dev=${DEV}@${thread}
>> +
>> +    # Add remove all other devices and add_device $dev to thread
>> +    pg_thread $thread "rem_device_all"
>> +    pg_thread $thread "add_device" $dev
>> +
>> +    # Base config of dev
>> +    pg_set $dev "flag QUEUE_MAP_CPU"
>> +    pg_set $dev "count $COUNT"
>> +    pg_set $dev "pkt_size $PKT_SIZE"
>> +    pg_set $dev "delay $DELAY"
>> +    pg_set $dev "flag NO_TIMESTAMP"
>> +
>> +    # Destination
>> +    pg_set $dev "dst_mac $DST_MAC"
>> +    pg_set $dev "dst $DEST_IP"
>> +
>> +    # Inject packet into RX path of stack
> 
> Hmmm, maybe above comment need to be adjusted...

Yep.

> 
>> +    pg_set $dev "xmit_mode queue_xmit"
>> +
>> +    # Burst allow us to avoid measuring SKB alloc/free overhead
> 
> This comment is confusing, maybe just remove. Didn't think burst is a
> valid use-case.

Yep.