From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Coquelin Subject: Re: [PATCH] examples/vhost_crypto: fix zero copy Date: Fri, 9 Nov 2018 12:40:20 +0100 Message-ID: <37c8fddf-a1de-0a71-3e85-8c7606014676@redhat.com> References: <20181030144852.43339-1-roy.fan.zhang@intel.com> <4c3cdfb6-7b79-5b89-286d-8e8cae56f755@ericsson.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit To: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= , Fan Zhang , dev@dpdk.org Return-path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 8A9F14C9C for ; Fri, 9 Nov 2018 12:40:24 +0100 (CET) In-Reply-To: <4c3cdfb6-7b79-5b89-286d-8e8cae56f755@ericsson.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >> --- >>   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 >>   #include >> +#include >>   #include >>   #include >>   #include >> @@ -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".