On 2/11/2020 7:57 PM, Slava Ovsiienko wrote:
> Hi, Ferruh
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yi...@intel.com>
>> Sent: Monday, February 10, 2020 16:02
>> To: Slava Ovsiienko <viachesl...@mellanox.com>; dev@dpdk.org
>> Cc: Thomas Monjalon <tho...@monjalon.net>;
>> bernard.iremon...@intel.com; sta...@dpdk.org
>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix txonly flow generation
>> entropy
>>
>> On 2/9/2020 5:02 PM, Viacheslav Ovsiienko wrote:
>>> The testpmd application in txonly forwarding mode has an option to
>>> generate the packet flows by varying the destination IP address.
>>> The patch increments the IP for each packet sent, this improves the
>>> entropy and RSS distibution on the peer receiving size is getting more
>>> uniform.
>>
>> The IP address already incremented for each packet sent [1], I can't see what
>> this patch adds.
>
> Not exactly, that commit increments the ip_var local variable only.
> On the next call ip_var is assigned from thread local storage variable
> ip_var = RTE_PER_LCORE(_ip_var) - and increment is lost, it is supposed to be
> a bug.
> We have the bursts of packets with the same IPs.
Ahh, current code assumes the 'pkt_burst_prepare()' done as burst. And tries to
save the increment to thread local storage in 'pkt_burst_transmit()', but since
'pkt_burst_prepare()' is per packet, this logic doesn't work.
And it looks like that logic was correct when implemented, but broken during
refactoring, can you please update fixes line accordingly:
Fixes: 01b645dcff7f ("app/testpmd: move txonly prepare in separate function")
>
> My patch stores the incremented value in the local storage variable. It is
> double checked,
> after applying the patch we see the more fair RSS distribution on receiving
> peer.
>
> With best regards, Slava
>
>>
>> [1]
>> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Flxr.dpdk.
>> org%2Fdpdk%2Fv19.11%2Fsource%2Fapp%2Ftest-
>> pmd%2Ftxonly.c%23L208&data=02%7C01%7Cviacheslavo%40mellanox.co
>> m%7C6a5be2d6ecdd4c487e7908d7ae31dc95%7Ca652971c7d2e4d9ba6a4d149
>> 256f461b%7C0%7C0%7C637169401491712071&sdata=QMXvKlSVy3vAUMz
>> Fzj%2ByRKKWUcVToR3hlX8S3a7V21I%3D&reserved=0
>>
>>
>> btw, for reference the option mentioned is "--txonly-multi-flow" and it has
>> been added with:
>> commit: 82010ef55e7c ("app/testpmd: make txonly mode generate multiple
>> flows")
>>
> Mmm, yes. My patch refers exactly to the this. Sorry, I do not understand
> your comment,
> could you, please, clarify what do you mean?
Just noting down for record.
>
> With best regards, Slava
>
