[ovs-dev] [PATCH v4 1/2] dpif-netdev: Add SMC cache after EMC cache

[Wang, Yipeng1]

Thanks for the comments, please see my reply inlined.

>I've checked the latest patch and the performance results I get are similar to the ones give in the previous patches. Also
>enabling/disabling the DFC on the fly works as expected.
>
>The main query I have regards the slow sweep for SMC
>
>[[BO'M]] The slow sweep removes EMC entries that are no longer valid because the associated dpcls rule has been changed or has
>expired. Is there a mechanism to remove SMC entries associated with a changed/expired dpcls rules?

[Wang, Yipeng] It is a good question. EMC needs to sweep is mostly because EMC holds pointers to the dp_netdev_flow. If EMC does not sweep and release the pointers, a dead dpcls rule cannot be de-allocated from memory as long as the EMC entry referencing it stays.  SMC does not hold pointers, so the dead dpcls can be de-allocated at any time. SMC holds an index to flow_table. If the entry in flow_table is removed, SMC will just find a miss (and it is OK since SMC is just a cache).  So functionally, slow sweeping is not needed.  However, sweeping SMC may periodically give new flows more empty entries in SMC. But this adds code complexity so we eventually decide to remove this part of code.

>>  }
>>
>> +/* Find a node by the index of the entry of cmap. For example, index 7
>> +means
>> + * the second bucket and the third item.
>[[BO'M]] Is this is assuming 4 for the bucket size. Maybe explicitly add where the bucket size is coming from - CMAP_K ? If so is that
>value not 5 for a 64 bit system?
>
[Wang, Yipeng] I will change the comment to use CMAP_K instead of the numbers as example.

>> +++ b/lib/dpif-netdev.c
>> @@ -129,7 +129,9 @@ struct netdev_flow_key {
>>      uint64_t buf[FLOW_MAX_PACKET_U64S];  };
>>
>> -/* Exact match cache for frequently used flows
>> +/* EMC cache and SMC cache compose of the datapath flow cache (DFC)
>[[BO'M]] 'compose of the' -> 'compose the' or else 'together make the'
>> @@ -2297,6 +2373,76 @@ emc_lookup(struct emc_cache *cache, const struct
>> netdev_flow_key *key)
>>      return NULL;
>>  }
>>
>> +static inline const struct cmap_node *
>> +smc_entry_get(struct dp_netdev_pmd_thread *pmd, struct smc_cache *cache,
>> +                const uint32_t hash)
>[[BO'M]] Minor quibble. We can get cache arg from pmd arg - pmd->flow_cache->smc_cache.

[Wang, Yipeng] I will fix all the issues you mentioned above.

Thanks!
Yipeng