From mboxrd@z Thu Jan 1 00:00:00 1970 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) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DF277307CDC9; Fri, 9 Nov 2018 11:40:23 +0000 (UTC) Received: from [10.36.112.39] (ovpn-112-39.ams2.redhat.com [10.36.112.39]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A93CF6013F; Fri, 9 Nov 2018 11:40:22 +0000 (UTC) To: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= , Fan Zhang , dev@dpdk.org References: <20181030144852.43339-1-roy.fan.zhang@intel.com> <4c3cdfb6-7b79-5b89-286d-8e8cae56f755@ericsson.com> From: Maxime Coquelin Message-ID: <37c8fddf-a1de-0a71-3e85-8c7606014676@redhat.com> Date: Fri, 9 Nov 2018 12:40:20 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <4c3cdfb6-7b79-5b89-286d-8e8cae56f755@ericsson.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Fri, 09 Nov 2018 11:40:23 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH] examples/vhost_crypto: fix zero copy X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Nov 2018 11:40:25 -0000 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".