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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3016f908-7c9f-6ed5-714e-686def88aae5@marvell.com>
Date:   Tue, 26 Jan 2021 17:10:13 +0100
From:   Igor Russkikh <irusskikh@...vell.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>,
        "Daniel T. Lee" <danieltimlee@...il.com>
CC:     <netdev@...r.kernel.org>, "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [EXT] Re: [PATCH net-next 2/2] samples: pktgen: new append mode

Hi Jesper,

Thanks for reviewing this.

>> DELAY may now be explicitly specified via common parameter -w
> 
> What are you actually using this for?

Basically, for the second patch.

When running multidev pktgen (using that -a option) with large amount of clones and
bursts (-c and -b) I saw that some of the devices got stuck - i.e. traffic was not
distributed evenly I think the reason of that is `next_to_run` selection logic, which
always takes first pkt_dev in a list.
Adding even a small delay param makes it consider next_tx data and creates a uniform
distribution between devices.

May be it makes sense to reconsider `next_to_run` selection logic, but I was
concentrating on scripts, so thats it.

> Notice there is also an option called "ratep" which can be used for
> setting the packet rate per sec.  In the pktgen.c code, it will use the
> "delay" variable.

Yes, I think in current form it makes no much harm if user knows what he wants.

>> +  -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.

Thanks, will fix.

>>  
>> -[[ $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:

Agree on style, could be fixed.

> 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).

I also don't like much the fact trap is being placed in that file.
Here I've placed one extra "-z $APPEND" exactly because of that.

In append mode of usage we do `source functions.sh` directly from bash.
That causes a side effect that trap is installed in root shell.
I can't check if thats APPEND mode or not at this moment. Thats why I do check APPEND
inside of the trap.

An alternative would be moving trap (or a function installing the trap) into each of
the scripts. That was the old behavior BTW.

>> +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)

Agreed, will fix.

Thanks,
  Igor