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
