Re: [dpdk-dev] [PATCH] examples/vhost_crypto: fix zero copy

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Fan,

Could you please have a look at Mattias comments and reply?

Thanks in advance,
Maxime

On 10/30/18 8:38 PM, Mattias Rönnblom wrote:
> On 2018-10-30 15:48, Fan Zhang wrote:
>> This patch fixes the zero copy enable problem for vhost crypto
>> sample application.
>>
>> For some Crypto PMDs such as AESNI-MB and AESNI-GCM the data to
>> be processed will be made a copy in the same buffer but next to the
>> data. For example, to encrypt 64 bytes data the PMD will copy this
>> data from offset 64 to offset 123. This requires the application
>> provides the buffer with at least double of the data size.
>>
>> However there is no way for VMs to know this limitation. When
>> zero-copy is enabled in Vhost the PMD may overwrite the buffer
>> next to the VM data to be processed, and further cause problems
>> such as Segmentation Fault or even worse, crashes the VM.
>>
>> To fix the problem the user should avoid enabling the zero copy
>> for these Crypto PMDs. This patch adds the checking of the PMD
>> names to see if zero copy can be applied.
>>
>> Fixes: 709521f4c2cd ("examples/vhost_crypto: support multi-core")
>>
>> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
>> ---
>>   examples/vhost_crypto/main.c | 23 +++++++++++++++++++++--
>>   1 file changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/examples/vhost_crypto/main.c b/examples/vhost_crypto/main.c
>> index cbb5e49d2..887e3eb6f 100644
>> --- a/examples/vhost_crypto/main.c
>> +++ b/examples/vhost_crypto/main.c
>> @@ -4,6 +4,7 @@
>>   #include <stdio.h>
>>   #include <stdlib.h>
>> +#include <string.h>
>>   #include <unistd.h>
>>   #include <stdbool.h>
>>   #include <assert.h>
>> @@ -442,8 +443,13 @@ free_resource(void)
>>           struct lcore_option *lo = &options.los[i];
>>           struct vhost_crypto_info *info = options.infos[i];
>> -        rte_mempool_free(info->cop_pool);
>> -        rte_mempool_free(info->sess_pool);
>> +        if (!info)
>> +            continue;
>> +
>> +        if (info->cop_pool)
>> +            rte_mempool_free(info->cop_pool);
>> +        if (info->sess_pool)
>> +            rte_mempool_free(info->sess_pool);
> 
> rte_mempool_free() already does a NULL-check (as per libc free() 
> convention), and if you are to do a NULL-check it should be an explicit 
> one ("!= NULL").
> 
>>           for (j = 0; j < lo->nb_sockets; j++) {
>>               rte_vhost_driver_unregister(lo->socket_files[i]);
>> @@ -493,6 +499,19 @@ main(int argc, char *argv[])
>>           info->nb_vids = lo->nb_sockets;
>>           rte_cryptodev_info_get(info->cid, &dev_info);
>> +        if (options.zero_copy == RTE_VHOST_CRYPTO_ZERO_COPY_ENABLE) {
>> +#define VHOST_CRYPTO_CDEV_NAME_AESNI_MB_PMD    crypto_aesni_mb
>> +#define VHOST_CRYPTO_CDEV_NAME_AESNI_GCM_PMD    crypto_aesni_gcm
> 
> What's the purpose of these defines?
> 
>> +            if (strstr(dev_info.driver_name,
>> +                RTE_STR(VHOST_CRYPTO_CDEV_NAME_AESNI_MB_PMD)) ||
>> +                strstr(dev_info.driver_name,
>> +                RTE_STR(VHOST_CRYPTO_CDEV_NAME_AESNI_GCM_PMD)))
>> +            RTE_LOG(ERR, USER1, "Cannot enable Zero Copy to %s\n",
>> +                    dev_info.driver_name);
> 
> "Zero Copy to" should probably be "zero-copy in" or "Zero-copy in".