[ovs-dev,v3] dpif-netdev: Fix insertion probability

emc_conditional_insert uses pmd->last_cycles and the packet's RSS hash
to generate a random number used to determine whether or not an emc
entry should be inserted. This works for single-packet bursts as
last_cycles is updated for each burst. However, for bursts > 1 packet,
where the packets in the batch generate the same RSS hash,
pmd->last_cycles remains constant for the entire burst also, and thus
cannot be used as a random number for each packet in the burst.

This commit replaces the use of pmd->last_cycles with random_uint32()
for this purpose and subsequently fixes the behavior of the
emc_insert_inv_prob setting for high-throughput (large bursts)
single-flow cases.

Fixes: 4c30b24602c3 ("dpif-netdev: Conditional EMC insert")
Reported-by: Kevin Traynor <ktraynor@redhat.com>
Acked-by: Darrell Ball <dlu998@gmail.com>
Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
---
v2:
- Remove unnecessary ORing in random number calcuation as per Ben's
  suggestion.
v3:
- Coding standards fix

 lib/dpif-netdev.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

On 06/23/2017 04:31 PM, Ciara Loftus wrote:
> emc_conditional_insert uses pmd->last_cycles and the packet's RSS hash
> to generate a random number used to determine whether or not an emc
> entry should be inserted. This works for single-packet bursts as
> last_cycles is updated for each burst. However, for bursts > 1 packet,
> where the packets in the batch generate the same RSS hash,
> pmd->last_cycles remains constant for the entire burst also, and thus
> cannot be used as a random number for each packet in the burst.
> 
> This commit replaces the use of pmd->last_cycles with random_uint32()
> for this purpose and subsequently fixes the behavior of the
> emc_insert_inv_prob setting for high-throughput (large bursts)
> single-flow cases.
> 

Thanks for the fix.
Acked-by: Kevin Traynor <ktraynor@redhat.com>

I also ran a quick test (on V2)
Tested-by: Kevin Traynor <ktraynor@redhat.com>

> Fixes: 4c30b24602c3 ("dpif-netdev: Conditional EMC insert")
> Reported-by: Kevin Traynor <ktraynor@redhat.com>
> Acked-by: Darrell Ball <dlu998@gmail.com>
> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> ---
> v2:
> - Remove unnecessary ORing in random number calcuation as per Ben's
>   suggestion.
> v3:
> - Coding standards fix
> 
>  lib/dpif-netdev.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 4e29085..2dbdd47 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2073,11 +2073,7 @@ emc_probabilistic_insert(struct dp_netdev_pmd_thread *pmd,
>      uint32_t min;
>      atomic_read_relaxed(&pmd->dp->emc_insert_min, &min);
>  
> -#ifdef DPDK_NETDEV
> -    if (min && (key->hash ^ (uint32_t) pmd->last_cycles) <= min) {
> -#else
> -    if (min && (key->hash ^ random_uint32()) <= min) {
> -#endif
> +    if (min && random_uint32() <= min) {
>          emc_insert(&pmd->flow_cache, key, flow);
>      }
>  }
>

On Fri, Jun 23, 2017 at 04:59:40PM +0100, Kevin Traynor wrote:
> On 06/23/2017 04:31 PM, Ciara Loftus wrote:
> > emc_conditional_insert uses pmd->last_cycles and the packet's RSS hash
> > to generate a random number used to determine whether or not an emc
> > entry should be inserted. This works for single-packet bursts as
> > last_cycles is updated for each burst. However, for bursts > 1 packet,
> > where the packets in the batch generate the same RSS hash,
> > pmd->last_cycles remains constant for the entire burst also, and thus
> > cannot be used as a random number for each packet in the burst.
> > 
> > This commit replaces the use of pmd->last_cycles with random_uint32()
> > for this purpose and subsequently fixes the behavior of the
> > emc_insert_inv_prob setting for high-throughput (large bursts)
> > single-flow cases.
> > 
> 
> Thanks for the fix.
> Acked-by: Kevin Traynor <ktraynor@redhat.com>
> 
> I also ran a quick test (on V2)
> Tested-by: Kevin Traynor <ktraynor@redhat.com>

Thanks Ciara, Kevin, Darrell.  I applied this to master.