Message ID | 1472725298-8455-1-git-send-email-john.griffin@intel.com (mailing list archive) |
---|---|
State | Superseded, archived |
Delegated to: | Pablo de Lara Guarch |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 2CD3F2BCD; Thu, 1 Sep 2016 12:21:45 +0200 (CEST) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 9C68F2946 for <dev@dpdk.org>; Thu, 1 Sep 2016 12:21:43 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga104.jf.intel.com with ESMTP; 01 Sep 2016 03:21:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,266,1470726000"; d="scan'208";a="163222430" Received: from sie-lab-212-151.ir.intel.com (HELO silpixa00390977.ir.intel.com) ([10.237.212.151]) by fmsmga004.fm.intel.com with ESMTP; 01 Sep 2016 03:21:41 -0700 From: John Griffin <john.griffin@intel.com> To: dev@dpdk.org Cc: eoin.breen@intel.com, pablo.de.lara.guarch@intel.com, John Griffin <john.griffin@intel.com> Date: Thu, 1 Sep 2016 11:21:38 +0100 Message-Id: <1472725298-8455-1-git-send-email-john.griffin@intel.com> X-Mailer: git-send-email 2.1.0 Subject: [dpdk-dev] [PATCH] crypto/qat: fix memzone creation to use a fixed size string X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Griffin, John
Sept. 1, 2016, 10:21 a.m. UTC
Remove the dependency on dev->driver->pci_drv.name when
creating the memzone for the qat hardware queues.
The pci_drv.name may grow too large for RTE_MEMZONE_NAMESIZE.
Fixes: 1703e94ac5ce ("qat: add driver for QuickAssist devices")
Signed-off-by: John Griffin <john.griffin@intel.com>
---
drivers/crypto/qat/qat_qp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On Thu, Sep 01, 2016 at 11:21:38AM +0100, John Griffin wrote: > Remove the dependency on dev->driver->pci_drv.name when > creating the memzone for the qat hardware queues. > The pci_drv.name may grow too large for RTE_MEMZONE_NAMESIZE. Will the "may grow too large" cause any issues? If so, state it here. If not, marking this patch as a "fix" patch doesn't make sense to me then. > > Fixes: 1703e94ac5ce ("qat: add driver for QuickAssist devices") > > Signed-off-by: John Griffin <john.griffin@intel.com> > --- > drivers/crypto/qat/qat_qp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/crypto/qat/qat_qp.c b/drivers/crypto/qat/qat_qp.c > index 5de47e3..a29ed66 100644 > --- a/drivers/crypto/qat/qat_qp.c > +++ b/drivers/crypto/qat/qat_qp.c > @@ -300,7 +300,7 @@ qat_queue_create(struct rte_cryptodev *dev, struct qat_queue *queue, > * Allocate a memzone for the queue - create a unique name. > */ > snprintf(queue->memz_name, sizeof(queue->memz_name), "%s_%s_%d_%d_%d", > - dev->driver->pci_drv.name, "qp_mem", dev->data->dev_id, > + "qat_pmd", "qp_mem", dev->data->dev_id, Besides that, why not putting "qat_pmd" and "qp_mem" inside the format string? --yliu
Hi Liu, Comments embedded. Rgds, John. On 05/09/16 04:23, Yuanhan Liu wrote: > On Thu, Sep 01, 2016 at 11:21:38AM +0100, John Griffin wrote: >> Remove the dependency on dev->driver->pci_drv.name when >> creating the memzone for the qat hardware queues. >> The pci_drv.name may grow too large for RTE_MEMZONE_NAMESIZE. > > Will the "may grow too large" cause any issues? If so, state it here. If > not, marking this patch as a "fix" patch doesn't make sense to me then. We discovered this when applying a future patch (2141c21966) and it exposed this issue. Problem is we create a memzone per hardware queue pair and if the memzone name is too large, then this code will not produce a unique name and two qps will end using the same memzone. In retrospect, I'll wait for Pablo to apply that future patch and then re-base a v2. > >> >> Fixes: 1703e94ac5ce ("qat: add driver for QuickAssist devices") >> >> Signed-off-by: John Griffin <john.griffin@intel.com> >> --- >> drivers/crypto/qat/qat_qp.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/crypto/qat/qat_qp.c b/drivers/crypto/qat/qat_qp.c >> index 5de47e3..a29ed66 100644 >> --- a/drivers/crypto/qat/qat_qp.c >> +++ b/drivers/crypto/qat/qat_qp.c >> @@ -300,7 +300,7 @@ qat_queue_create(struct rte_cryptodev *dev, struct qat_queue *queue, >> * Allocate a memzone for the queue - create a unique name. >> */ >> snprintf(queue->memz_name, sizeof(queue->memz_name), "%s_%s_%d_%d_%d", >> - dev->driver->pci_drv.name, "qp_mem", dev->data->dev_id, >> + "qat_pmd", "qp_mem", dev->data->dev_id, > > Besides that, why not putting "qat_pmd" and "qp_mem" inside the format > string? Fair point will send in the v2. > > --yliu >
On Wed, Sep 14, 2016 at 04:32:46PM +0100, John Griffin wrote: > Hi Liu, > Comments embedded. > > Rgds, > John. > > On 05/09/16 04:23, Yuanhan Liu wrote: > >On Thu, Sep 01, 2016 at 11:21:38AM +0100, John Griffin wrote: > >>Remove the dependency on dev->driver->pci_drv.name when > >>creating the memzone for the qat hardware queues. > >>The pci_drv.name may grow too large for RTE_MEMZONE_NAMESIZE. > > > >Will the "may grow too large" cause any issues? If so, state it here. If > >not, marking this patch as a "fix" patch doesn't make sense to me then. > We discovered this when applying a future patch (2141c21966) and it exposed > this issue. > Problem is we create a memzone per hardware queue pair and if the memzone > name is too large, then this code will not produce a unique > name and two qps will end using the same memzone. Thanks for the info, and I think you should put it in the commit log: it helps people to really know what might go wrong without this fix. --yliu
On 18/09/16 09:16, Yuanhan Liu wrote: > On Wed, Sep 14, 2016 at 04:32:46PM +0100, John Griffin wrote: >> Hi Liu, >> Comments embedded. >> >> Rgds, >> John. >> >> On 05/09/16 04:23, Yuanhan Liu wrote: >>> On Thu, Sep 01, 2016 at 11:21:38AM +0100, John Griffin wrote: >>>> Remove the dependency on dev->driver->pci_drv.name when >>>> creating the memzone for the qat hardware queues. >>>> The pci_drv.name may grow too large for RTE_MEMZONE_NAMESIZE. >>> >>> Will the "may grow too large" cause any issues? If so, state it here. If >>> not, marking this patch as a "fix" patch doesn't make sense to me then. >> We discovered this when applying a future patch (2141c21966) and it exposed >> this issue. >> Problem is we create a memzone per hardware queue pair and if the memzone >> name is too large, then this code will not produce a unique >> name and two qps will end using the same memzone. > > Thanks for the info, and I think you should put it in the commit log: it > helps people to really know what might go wrong without this fix. > > --yliu > No problem. Yes will add to the v2. Rgds, John.
diff --git a/drivers/crypto/qat/qat_qp.c b/drivers/crypto/qat/qat_qp.c index 5de47e3..a29ed66 100644 --- a/drivers/crypto/qat/qat_qp.c +++ b/drivers/crypto/qat/qat_qp.c @@ -300,7 +300,7 @@ qat_queue_create(struct rte_cryptodev *dev, struct qat_queue *queue, * Allocate a memzone for the queue - create a unique name. */ snprintf(queue->memz_name, sizeof(queue->memz_name), "%s_%s_%d_%d_%d", - dev->driver->pci_drv.name, "qp_mem", dev->data->dev_id, + "qat_pmd", "qp_mem", dev->data->dev_id, queue->hw_bundle_number, queue->hw_queue_number); qp_mz = queue_dma_zone_reserve(queue->memz_name, queue_size_bytes, socket_id);