Patch Comment List
List patch comments
GET /api/patches/40707/comments/?format=api
[ { "id": 82076, "web_url": "http://patches.dpdk.org/comment/82076/", "msgid": "<20180612063514.GA14993@debian>", "list_archive_url": "https://inbox.dpdk.org/dev/20180612063514.GA14993@debian", "date": "2018-06-12T06:35:14", "subject": "Re: [dpdk-dev] [PATCH v3 1/5] net/virtio: forbid simple Tx path by\n\tdefault", "submitter": { "id": 617, "url": "http://patches.dpdk.org/api/people/617/?format=api", "name": "Tiwei Bie", "email": "tiwei.bie@intel.com" }, "content": "On Thu, Jun 07, 2018 at 11:26:12AM +0200, Maxime Coquelin wrote:\n> Simple Tx path is not compliant with the Virtio specification,\n> as it assumes the device will use the descriptors in order.\n> \n> VIRTIO_F_IN_ORDER feature has been introduced recently, but the\n> simple Tx path is not compliant with it as VIRTIO_F_IN_ORDER\n> requires that chained descriptors are used sequentially, which\n> is not the case in simple Tx path.\n> \n> This patch introduces 'simple_tx_support' devarg to unlock\n> Tx simple path selection.\n> \n> Reported-by: Tiwei Bie <tiwei.bie@intel.com>\n> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>\n> ---\n> doc/guides/nics/virtio.rst | 9 +++++\n> drivers/net/virtio/virtio_ethdev.c | 73 +++++++++++++++++++++++++++++++++++++-\n> drivers/net/virtio/virtio_pci.h | 1 +\n> 3 files changed, 82 insertions(+), 1 deletion(-)\n> \n> diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst\n> index 8922f9c0b..53ce1c12a 100644\n> --- a/doc/guides/nics/virtio.rst\n> +++ b/doc/guides/nics/virtio.rst\n> @@ -222,6 +222,9 @@ Tx callbacks:\n> \n> #. ``virtio_xmit_pkts_simple``:\n> Vector version fixes the available ring indexes to optimize performance.\n> + This implementation does not comply with the Virtio specification, and so\n> + is not selectable by default. \"simple_tx_support=1\" devarg must be passed\n> + to unlock it.\n> \n> \n> By default, the non-vector callbacks are used:\n> @@ -331,3 +334,9 @@ The user can specify below argument in devargs.\n> driver, and works as a HW vhost backend. This argument is used to specify\n> a virtio device needs to work in vDPA mode.\n> (Default: 0 (disabled))\n> +\n> +#. ``simple_tx_support``:\n> +\n> + This argument enables support for the simple Tx path, which is not\n> + compliant with the Virtio specification.\n> + (Default: 0 (disabled))\n\nI tried this patch on my server. Virtio-user will\nfail to probe when simple_tx_support is specified\ndues to the check in virtio_user_pmd_probe():\n\nPMD: Error parsing device, invalid key <simple_tx_support>\nvirtio_user_pmd_probe(): error when parsing param\nvdev_probe(): failed to initialize virtio_user0 device\nEAL: Bus (vdev) probe failed.\n\n\n> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c\n> index 5833dad73..052dd056a 100644\n> --- a/drivers/net/virtio/virtio_ethdev.c\n> +++ b/drivers/net/virtio/virtio_ethdev.c\n> @@ -1331,6 +1331,8 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)\n> \tif (hw->use_simple_tx) {\n> \t\tPMD_INIT_LOG(INFO, \"virtio: using simple Tx path on port %u\",\n> \t\t\teth_dev->data->port_id);\n> +\t\tPMD_INIT_LOG(WARNING,\n> +\t\t\t\t\"virtio: simple Tx path does not comply with Virtio spec\");\n> \t\teth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;\n> \t} else {\n> \t\tPMD_INIT_LOG(INFO, \"virtio: using standard Tx path on port %u\",\n> @@ -1790,6 +1792,66 @@ rte_virtio_pmd_init(void)\n> \trte_pci_register(&rte_virtio_pmd);\n> }\n> \n> +#define VIRTIO_SIMPLE_TX_SUPPORT \"simple_tx_support\"\n> +\n> +static int virtio_dev_args_check(const char *key, const char *val,\n> +\t\tvoid *opaque)\n> +{\n> +\tstruct rte_eth_dev *dev = opaque;\n> +\tstruct virtio_hw *hw = dev->data->dev_private;\n> +\tunsigned long tmp;\n> +\tint ret = 0;\n> +\n> +\terrno = 0;\n> +\ttmp = strtoul(val, NULL, 0);\n> +\tif (errno) {\n> +\t\tPMD_INIT_LOG(INFO,\n> +\t\t\t\t\"%s: \\\"%s\\\" is not a valid integer\", key, val);\n> +\t\treturn errno;\n> +\t}\n> +\n> +\tif (strcmp(VIRTIO_SIMPLE_TX_SUPPORT, key) == 0)\n> +\t\thw->support_simple_tx = !!tmp;\n> +\n> +\treturn ret;\n> +}\n> +\n> +static int\n> +virtio_dev_args(struct rte_eth_dev *dev)\n> +{\n> +\tstruct rte_kvargs *kvlist;\n> +\tstruct rte_devargs *devargs;\n> +\tconst char *valid_args[] = {\n> +\t\tVIRTIO_SIMPLE_TX_SUPPORT,\n> +\t\tNULL,\n> +\t};\n\ncheckpatch is complaining about above definition:\n\nWARNING:STATIC_CONST_CHAR_ARRAY: char * array declaration might be\nbetter as static const\n#96: FILE: drivers/net/virtio/virtio_ethdev.c:1824:\n+ const char *valid_args[] = {\n\nBest regards,\nTiwei Bie", "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 [127.0.0.1])\n\tby dpdk.org (Postfix) with ESMTP id 6CF241E553;\n\tTue, 12 Jun 2018 08:35:10 +0200 (CEST)", "from mga04.intel.com (mga04.intel.com [192.55.52.120])\n\tby dpdk.org (Postfix) with ESMTP id A6BF41E4E1\n\tfor <dev@dpdk.org>; Tue, 12 Jun 2018 08:35:09 +0200 (CEST)", "from fmsmga002.fm.intel.com ([10.253.24.26])\n\tby fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t11 Jun 2018 23:35:07 -0700", "from debian.sh.intel.com (HELO debian) ([10.67.104.228])\n\tby fmsmga002.fm.intel.com with ESMTP; 11 Jun 2018 23:35:07 -0700" ], "X-Amp-Result": "UNKNOWN", "X-Amp-Original-Verdict": "FILE UNKNOWN", "X-Amp-File-Uploaded": "False", "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos;i=\"5.51,213,1526367600\"; d=\"scan'208\";a=\"56382278\"", "Date": "Tue, 12 Jun 2018 14:35:14 +0800", "From": "Tiwei Bie <tiwei.bie@intel.com>", "To": "Maxime Coquelin <maxime.coquelin@redhat.com>", "Cc": "zhihong.wang@intel.com, dev@dpdk.org", "Message-ID": "<20180612063514.GA14993@debian>", "References": "<20180607092616.27720-1-maxime.coquelin@redhat.com>\n\t<20180607092616.27720-2-maxime.coquelin@redhat.com>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=utf-8", "Content-Disposition": "inline", "In-Reply-To": "<20180607092616.27720-2-maxime.coquelin@redhat.com>", "User-Agent": "Mutt/1.9.5 (2018-04-13)", "Subject": "Re: [dpdk-dev] [PATCH v3 1/5] net/virtio: forbid simple Tx path by\n\tdefault", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.15", "Precedence": "list", "List-Id": "DPDK patches and discussions <dev.dpdk.org>", "List-Unsubscribe": "<https://dpdk.org/ml/options/dev>,\n\t<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": "<https://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org", "Sender": "\"dev\" <dev-bounces@dpdk.org>" }, "addressed": null }, { "id": 82088, "web_url": "http://patches.dpdk.org/comment/82088/", "msgid": "<d2c0a9d0-5821-c287-fe99-bf232ce91d10@redhat.com>", "list_archive_url": "https://inbox.dpdk.org/dev/d2c0a9d0-5821-c287-fe99-bf232ce91d10@redhat.com", "date": "2018-06-12T11:52:02", "subject": "Re: [dpdk-dev] [PATCH v3 1/5] net/virtio: forbid simple Tx path by\n\tdefault", "submitter": { "id": 512, "url": "http://patches.dpdk.org/api/people/512/?format=api", "name": "Maxime Coquelin", "email": "maxime.coquelin@redhat.com" }, "content": "On 06/12/2018 08:35 AM, Tiwei Bie wrote:\n> On Thu, Jun 07, 2018 at 11:26:12AM +0200, Maxime Coquelin wrote:\n>> Simple Tx path is not compliant with the Virtio specification,\n>> as it assumes the device will use the descriptors in order.\n>>\n>> VIRTIO_F_IN_ORDER feature has been introduced recently, but the\n>> simple Tx path is not compliant with it as VIRTIO_F_IN_ORDER\n>> requires that chained descriptors are used sequentially, which\n>> is not the case in simple Tx path.\n>>\n>> This patch introduces 'simple_tx_support' devarg to unlock\n>> Tx simple path selection.\n>>\n>> Reported-by: Tiwei Bie <tiwei.bie@intel.com>\n>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>\n>> ---\n>> doc/guides/nics/virtio.rst | 9 +++++\n>> drivers/net/virtio/virtio_ethdev.c | 73 +++++++++++++++++++++++++++++++++++++-\n>> drivers/net/virtio/virtio_pci.h | 1 +\n>> 3 files changed, 82 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst\n>> index 8922f9c0b..53ce1c12a 100644\n>> --- a/doc/guides/nics/virtio.rst\n>> +++ b/doc/guides/nics/virtio.rst\n>> @@ -222,6 +222,9 @@ Tx callbacks:\n>> \n>> #. ``virtio_xmit_pkts_simple``:\n>> Vector version fixes the available ring indexes to optimize performance.\n>> + This implementation does not comply with the Virtio specification, and so\n>> + is not selectable by default. \"simple_tx_support=1\" devarg must be passed\n>> + to unlock it.\n>> \n>> \n>> By default, the non-vector callbacks are used:\n>> @@ -331,3 +334,9 @@ The user can specify below argument in devargs.\n>> driver, and works as a HW vhost backend. This argument is used to specify\n>> a virtio device needs to work in vDPA mode.\n>> (Default: 0 (disabled))\n>> +\n>> +#. ``simple_tx_support``:\n>> +\n>> + This argument enables support for the simple Tx path, which is not\n>> + compliant with the Virtio specification.\n>> + (Default: 0 (disabled))\n> \n> I tried this patch on my server. Virtio-user will\n> fail to probe when simple_tx_support is specified\n> dues to the check in virtio_user_pmd_probe():\n> \n> PMD: Error parsing device, invalid key <simple_tx_support>\n> virtio_user_pmd_probe(): error when parsing param\n> vdev_probe(): failed to initialize virtio_user0 device\n> EAL: Bus (vdev) probe failed.\n\n\nThanks for the heads-up, I hadn't tried with virtio-user.\nI'll post a new version with this fixed soon.\n\n> \n>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c\n>> index 5833dad73..052dd056a 100644\n>> --- a/drivers/net/virtio/virtio_ethdev.c\n>> +++ b/drivers/net/virtio/virtio_ethdev.c\n>> @@ -1331,6 +1331,8 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)\n>> \tif (hw->use_simple_tx) {\n>> \t\tPMD_INIT_LOG(INFO, \"virtio: using simple Tx path on port %u\",\n>> \t\t\teth_dev->data->port_id);\n>> +\t\tPMD_INIT_LOG(WARNING,\n>> +\t\t\t\t\"virtio: simple Tx path does not comply with Virtio spec\");\n>> \t\teth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;\n>> \t} else {\n>> \t\tPMD_INIT_LOG(INFO, \"virtio: using standard Tx path on port %u\",\n>> @@ -1790,6 +1792,66 @@ rte_virtio_pmd_init(void)\n>> \trte_pci_register(&rte_virtio_pmd);\n>> }\n>> \n>> +#define VIRTIO_SIMPLE_TX_SUPPORT \"simple_tx_support\"\n>> +\n>> +static int virtio_dev_args_check(const char *key, const char *val,\n>> +\t\tvoid *opaque)\n>> +{\n>> +\tstruct rte_eth_dev *dev = opaque;\n>> +\tstruct virtio_hw *hw = dev->data->dev_private;\n>> +\tunsigned long tmp;\n>> +\tint ret = 0;\n>> +\n>> +\terrno = 0;\n>> +\ttmp = strtoul(val, NULL, 0);\n>> +\tif (errno) {\n>> +\t\tPMD_INIT_LOG(INFO,\n>> +\t\t\t\t\"%s: \\\"%s\\\" is not a valid integer\", key, val);\n>> +\t\treturn errno;\n>> +\t}\n>> +\n>> +\tif (strcmp(VIRTIO_SIMPLE_TX_SUPPORT, key) == 0)\n>> +\t\thw->support_simple_tx = !!tmp;\n>> +\n>> +\treturn ret;\n>> +}\n>> +\n>> +static int\n>> +virtio_dev_args(struct rte_eth_dev *dev)\n>> +{\n>> +\tstruct rte_kvargs *kvlist;\n>> +\tstruct rte_devargs *devargs;\n>> +\tconst char *valid_args[] = {\n>> +\t\tVIRTIO_SIMPLE_TX_SUPPORT,\n>> +\t\tNULL,\n>> +\t};\n> \n> checkpatch is complaining about above definition:\n> \n> WARNING:STATIC_CONST_CHAR_ARRAY: char * array declaration might be\n> better as static const\n> #96: FILE: drivers/net/virtio/virtio_ethdev.c:1824:\n> + const char *valid_args[] = {\n\nYeah, I missed the warning when running my build scripts and noticed\nit from the upstream CI mail.\n\nIt will be fixed in next version.\n\nThanks!\nMaxime\n\n> Best regards,\n> Tiwei Bie\n>", "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 [127.0.0.1])\n\tby dpdk.org (Postfix) with ESMTP id DDD411E9C3;\n\tTue, 12 Jun 2018 13:52:06 +0200 (CEST)", "from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73])\n\tby dpdk.org (Postfix) with ESMTP id 621CF1E9C2\n\tfor <dev@dpdk.org>; Tue, 12 Jun 2018 13:52:05 +0200 (CEST)", "from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id BB72580401A8;\n\tTue, 12 Jun 2018 11:52:04 +0000 (UTC)", "from [10.36.112.45] (ovpn-112-45.ams2.redhat.com [10.36.112.45])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id E2BE3111CB82;\n\tTue, 12 Jun 2018 11:52:03 +0000 (UTC)" ], "To": "Tiwei Bie <tiwei.bie@intel.com>", "Cc": "zhihong.wang@intel.com, dev@dpdk.org", "References": "<20180607092616.27720-1-maxime.coquelin@redhat.com>\n\t<20180607092616.27720-2-maxime.coquelin@redhat.com>\n\t<20180612063514.GA14993@debian>", "From": "Maxime Coquelin <maxime.coquelin@redhat.com>", "Message-ID": "<d2c0a9d0-5821-c287-fe99-bf232ce91d10@redhat.com>", "Date": "Tue, 12 Jun 2018 13:52:02 +0200", "User-Agent": "Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.7.0", "MIME-Version": "1.0", "In-Reply-To": "<20180612063514.GA14993@debian>", "Content-Type": "text/plain; charset=utf-8; format=flowed", "Content-Language": "en-US", "Content-Transfer-Encoding": "7bit", "X-Scanned-By": "MIMEDefang 2.78 on 10.11.54.3", "X-Greylist": [ "Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.11.55.8]); Tue, 12 Jun 2018 11:52:04 +0000 (UTC)", "inspected by milter-greylist-4.5.16 (mx1.redhat.com\n\t[10.11.55.8]); \n\tTue, 12 Jun 2018 11:52:04 +0000 (UTC) for IP:'10.11.54.3'\n\tDOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com'\n\tHELO:'smtp.corp.redhat.com' FROM:'maxime.coquelin@redhat.com' RCPT:''" ], "Subject": "Re: [dpdk-dev] [PATCH v3 1/5] net/virtio: forbid simple Tx path by\n\tdefault", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.15", "Precedence": "list", "List-Id": "DPDK patches and discussions <dev.dpdk.org>", "List-Unsubscribe": "<https://dpdk.org/ml/options/dev>,\n\t<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": "<https://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org", "Sender": "\"dev\" <dev-bounces@dpdk.org>" }, "addressed": null } ]