Patch Comment List
List patch comments
GET /api/patches/126/comments/?format=api
[ { "id": 296, "web_url": "https://patches.dpdk.org/comment/296/", "msgid": "<20140805203254.2240a09c@haswell.linuxnetplumber.net>", "list_archive_url": "https://inbox.dpdk.org/dev/20140805203254.2240a09c@haswell.linuxnetplumber.net", "date": "2014-08-06T03:32:54", "subject": "Re: [dpdk-dev] [PATCH 3/3] examples/vhost: add new vhost example", "submitter": { "id": 27, "url": "https://patches.dpdk.org/api/people/27/?format=api", "name": "Stephen Hemminger", "email": "stephen@networkplumber.org" }, "content": "> +static const uint16_t external_pkt_default_vlan_tag = 2000;\n> +const uint16_t vlan_tags[] = {\n> +\t1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007,\n> +\t1008, 1009, 1010, 1011,\t1012, 1013, 1014, 1015,\n> +\t1016, 1017, 1018, 1019, 1020, 1021, 1022, 1023,\n> +\t1024, 1025, 1026, 1027, 1028, 1029, 1030, 1031,\n> +\t1032, 1033, 1034, 1035, 1036, 1037, 1038, 1039,\n> +\t1040, 1041, 1042, 1043, 1044, 1045, 1046, 1047,\n> +\t1048, 1049, 1050, 1051, 1052, 1053, 1054, 1055,\n> +\t1056, 1057, 1058, 1059, 1060, 1061, 1062, 1063,\n> +};\n\nWhy pre-compute table if it is just: vlan_tag = n + 1000?\n\n\n> +\n> +/* Per-device statistics struct */\n> +struct device_statistics {\n> +\tuint64_t tx_total;\n> +\trte_atomic64_t rx_total_atomic;\n> +\tuint64_t rx_total;\n> +\tuint64_t tx;\n> +\trte_atomic64_t rx_atomic;\n> +\tuint64_t rx;\n> +} __rte_cache_aligned;\n> +struct device_statistics dev_statistics[MAX_DEVICES];\n\nDoing per-core statistics would be faster than using atomic's\nwhich have implicit lock.\n\n> +/*\n> + * Builds up the correct configuration for VMDQ VLAN pool map\n> + * according to the pool & queue limits.\n> + */\n> +static inline int\n> +get_eth_conf(struct rte_eth_conf *eth_conf, uint32_t num_devices)\n> +{\n> +\tstruct rte_eth_vmdq_rx_conf conf;\n> +\tunsigned i;\n> +\n> +\tmemset(&conf, 0, sizeof(conf));\n> +\tconf.nb_queue_pools = (enum rte_eth_nb_pools)num_devices;\n> +\tconf.nb_pool_maps = num_devices;\n> +\tconf.enable_loop_back =\n> +\t\tvmdq_conf_default.rx_adv_conf.vmdq_rx_conf.enable_loop_back;\n> +\n> +\tfor (i = 0; i < conf.nb_pool_maps; i++) {\n> +\t\tconf.pool_map[i].vlan_id = vlan_tags[i];\n> +\t\tconf.pool_map[i].pools = (1UL << i);\n> +\t}\n> +\n> +\t(void)(rte_memcpy(eth_conf, &vmdq_conf_default, sizeof(*eth_conf)));\n> +\t(void)(rte_memcpy(ð_conf->rx_adv_conf.vmdq_rx_conf, &conf,\n> +\t\t sizeof(eth_conf->rx_adv_conf.vmdq_rx_conf)));\n> +\treturn 0;\n> +}\n\nIf function always returns 0 just make it void.\nNo point in making non-fastpath code inline.\nThe cast to (void) is unnecessary and just makes things ugly.\nWhy not use structure assignment rather than memcpy() which is not type safe?", "headers": { "Return-Path": "<stephen@networkplumber.org>", "Received": [ "from mail-pa0-f46.google.com (mail-pa0-f46.google.com\n\t[209.85.220.46]) by dpdk.org (Postfix) with ESMTP id D1F872E8A\n\tfor <dev@dpdk.org>; Wed, 6 Aug 2014 05:30:32 +0200 (CEST)", "by mail-pa0-f46.google.com with SMTP id lj1so2561175pab.19\n\tfor <dev@dpdk.org>; Tue, 05 Aug 2014 20:32:58 -0700 (PDT)", "from haswell.linuxnetplumber.net\n\t(static-50-53-72-226.bvtn.or.frontiernet.net. [50.53.72.226])\n\tby mx.google.com with ESMTPSA id\n\toy3sm5352535pdb.79.2014.08.05.20.32.57 for <multiple recipients>\n\t(version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tTue, 05 Aug 2014 20:32:57 -0700 (PDT)" ], "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20130820;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to\n\t:references:mime-version:content-type:content-transfer-encoding;\n\tbh=AnRZIkSJHai6+jomQcvfCNA01PJLER3tW3ZayEwJ+ko=;\n\tb=cpqxT+qCJzK89jQ8dJfHQ/Tl7Q+O3HK3BAOs7o6wz3IBIKRh8dUzXr5uXgbZMt6Lof\n\tCgE11I6NU1c3WkH/ddfb3GTI4ztmlZwGVP2vM1E0QpFwwxbnbIm/2y8LBZNFfcczdhqd\n\tdbbigbZa5Vfbika/NIgqI2H8p7QP7He52oFeQ5d+xT7ams4Wh7nvzmSB8Zxp9VgYnawU\n\tGx0hc3vI/EnLZeNPWXoKhip6XGKa7VUf5WOy/csYuFObGHZjPjD5JeJnf8KBEw8OEpOt\n\tFuRvUIq32gWkXf5eKyoiQbpZWSyXA9zYbmMLeZOHiH35KHMKU4A3Yuzc+tvR/ajVZNP7\n\teV2w==", "X-Gm-Message-State": "ALoCoQn42/v95+2v7U/2QibKxVeykzmlUpB9o2CQwafrvq4BIWfp8fAmGdGw1QISrNZ6/4ysUlVK", "X-Received": "by 10.68.131.134 with SMTP id om6mr8533207pbb.41.1407295977988; \n\tTue, 05 Aug 2014 20:32:57 -0700 (PDT)", "Date": "Tue, 5 Aug 2014 20:32:54 -0700", "From": "Stephen Hemminger <stephen@networkplumber.org>", "To": "Huawei Xie <huawei.xie@intel.com>", "Message-ID": "<20140805203254.2240a09c@haswell.linuxnetplumber.net>", "In-Reply-To": "<1407254286-23972-4-git-send-email-huawei.xie@intel.com>", "References": "<1407254286-23972-1-git-send-email-huawei.xie@intel.com>\n\t<1407254286-23972-4-git-send-email-huawei.xie@intel.com>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=US-ASCII", "Content-Transfer-Encoding": "7bit", "Cc": "dev@dpdk.org", "Subject": "Re: [dpdk-dev] [PATCH 3/3] examples/vhost: add new vhost example", "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>,\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": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "X-List-Received-Date": "Wed, 06 Aug 2014 03:30:33 -0000" }, "addressed": null }, { "id": 297, "web_url": "https://patches.dpdk.org/comment/297/", "msgid": "<C37D651A908B024F974696C65296B57B0F25B105@SHSMSX101.ccr.corp.intel.com>", "list_archive_url": "https://inbox.dpdk.org/dev/C37D651A908B024F974696C65296B57B0F25B105@SHSMSX101.ccr.corp.intel.com", "date": "2014-08-06T05:34:44", "subject": "Re: [dpdk-dev] [PATCH 3/3] examples/vhost: add new vhost example", "submitter": { "id": 16, "url": "https://patches.dpdk.org/api/people/16/?format=api", "name": "Huawei Xie", "email": "huawei.xie@intel.com" }, "content": "> -----Original Message-----\n> From: Stephen Hemminger [mailto:stephen@networkplumber.org]\n> Sent: Wednesday, August 06, 2014 11:33 AM\n> To: Xie, Huawei\n> Cc: dev@dpdk.org\n> Subject: Re: [dpdk-dev] [PATCH 3/3] examples/vhost: add new vhost example\n> \n> \n> > +static const uint16_t external_pkt_default_vlan_tag = 2000;\n> > +const uint16_t vlan_tags[] = {\n> > +\t1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007,\n> > +\t1008, 1009, 1010, 1011,\t1012, 1013, 1014, 1015,\n> > +\t1016, 1017, 1018, 1019, 1020, 1021, 1022, 1023,\n> > +\t1024, 1025, 1026, 1027, 1028, 1029, 1030, 1031,\n> > +\t1032, 1033, 1034, 1035, 1036, 1037, 1038, 1039,\n> > +\t1040, 1041, 1042, 1043, 1044, 1045, 1046, 1047,\n> > +\t1048, 1049, 1050, 1051, 1052, 1053, 1054, 1055,\n> > +\t1056, 1057, 1058, 1059, 1060, 1061, 1062, 1063,\n> > +};\n> \n> Why pre-compute table if it is just: vlan_tag = n + 1000?\nThis is inherited from old vhost example. For demonstration purpose, I feel it is ok. Besides, the purpose of this patch is to refactor the example using vhost library, so I prefer subsequent patches to fix those kind of problems.\n> \n> \n> > +\n> > +/* Per-device statistics struct */\n> > +struct device_statistics {\n> > +\tuint64_t tx_total;\n> > +\trte_atomic64_t rx_total_atomic;\n> > +\tuint64_t rx_total;\n> > +\tuint64_t tx;\n> > +\trte_atomic64_t rx_atomic;\n> > +\tuint64_t rx;\n> > +} __rte_cache_aligned;\n> > +struct device_statistics dev_statistics[MAX_DEVICES];\n> \n> Doing per-core statistics would be faster than using atomic's\n> which have implicit lock.\nFor software vm2vm mode, there is contention as two cores could enqueue the same virtio ring concurrently.\n> \n> > +/*\n> > + * Builds up the correct configuration for VMDQ VLAN pool map\n> > + * according to the pool & queue limits.\n> > + */\n> > +static inline int\n> > +get_eth_conf(struct rte_eth_conf *eth_conf, uint32_t num_devices)\n> > +{\n> > +\tstruct rte_eth_vmdq_rx_conf conf;\n> > +\tunsigned i;\n> > +\n> > +\tmemset(&conf, 0, sizeof(conf));\n> > +\tconf.nb_queue_pools = (enum rte_eth_nb_pools)num_devices;\n> > +\tconf.nb_pool_maps = num_devices;\n> > +\tconf.enable_loop_back =\n> > +\n> \tvmdq_conf_default.rx_adv_conf.vmdq_rx_conf.enable_loop_back;\n> > +\n> > +\tfor (i = 0; i < conf.nb_pool_maps; i++) {\n> > +\t\tconf.pool_map[i].vlan_id = vlan_tags[i];\n> > +\t\tconf.pool_map[i].pools = (1UL << i);\n> > +\t}\n> > +\n> > +\t(void)(rte_memcpy(eth_conf, &vmdq_conf_default, sizeof(*eth_conf)));\n> > +\t(void)(rte_memcpy(ð_conf->rx_adv_conf.vmdq_rx_conf, &conf,\n> > +\t\t sizeof(eth_conf->rx_adv_conf.vmdq_rx_conf)));\n> > +\treturn 0;\n> > +}\n> \n> If function always returns 0 just make it void.\n> No point in making non-fastpath code inline.\n> The cast to (void) is unnecessary and just makes things ugly.\n> Why not use structure assignment rather than memcpy() which is not type safe?\nAgree. As stated above, I prefer subsequent patches to fix those issues inherited from old example. How do you think?", "headers": { "Return-Path": "<huawei.xie@intel.com>", "Received": [ "from mga09.intel.com (mga09.intel.com [134.134.136.24])\n\tby dpdk.org (Postfix) with ESMTP id 8A40B594E\n\tfor <dev@dpdk.org>; Wed, 6 Aug 2014 07:32:22 +0200 (CEST)", "from orsmga002.jf.intel.com ([10.7.209.21])\n\tby orsmga102.jf.intel.com with ESMTP; 05 Aug 2014 22:29:00 -0700", "from fmsmsx103.amr.corp.intel.com ([10.19.9.34])\n\tby orsmga002.jf.intel.com with ESMTP; 05 Aug 2014 22:34:47 -0700", "from fmsmsx157.amr.corp.intel.com (10.18.116.73) by\n\tFMSMSX103.amr.corp.intel.com (10.19.9.34) with Microsoft SMTP Server\n\t(TLS) id 14.3.123.3; Tue, 5 Aug 2014 22:34:46 -0700", "from shsmsx152.ccr.corp.intel.com (10.239.6.52) by\n\tFMSMSX157.amr.corp.intel.com (10.18.116.73) with Microsoft SMTP\n\tServer (TLS) id 14.3.123.3; Tue, 5 Aug 2014 22:34:46 -0700", "from shsmsx101.ccr.corp.intel.com ([169.254.1.252]) by\n\tSHSMSX152.ccr.corp.intel.com ([169.254.6.217]) with mapi id\n\t14.03.0195.001; Wed, 6 Aug 2014 13:34:44 +0800" ], "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos;i=\"5.01,810,1400050800\"; d=\"scan'208\";a=\"584039144\"", "From": "\"Xie, Huawei\" <huawei.xie@intel.com>", "To": "Stephen Hemminger <stephen@networkplumber.org>", "Thread-Topic": "[dpdk-dev] [PATCH 3/3] examples/vhost: add new vhost example", "Thread-Index": "AQHPsSctpEDAB4zYak+DKZ9INDld/pvDChGw", "Date": "Wed, 6 Aug 2014 05:34:44 +0000", "Message-ID": "<C37D651A908B024F974696C65296B57B0F25B105@SHSMSX101.ccr.corp.intel.com>", "References": "<1407254286-23972-1-git-send-email-huawei.xie@intel.com>\n\t<1407254286-23972-4-git-send-email-huawei.xie@intel.com>\n\t<20140805203254.2240a09c@haswell.linuxnetplumber.net>", "In-Reply-To": "<20140805203254.2240a09c@haswell.linuxnetplumber.net>", "Accept-Language": "en-US", "Content-Language": "en-US", "X-MS-Has-Attach": "", "X-MS-TNEF-Correlator": "", "x-originating-ip": "[10.239.127.40]", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "quoted-printable", "MIME-Version": "1.0", "Cc": "\"dev@dpdk.org\" <dev@dpdk.org>", "Subject": "Re: [dpdk-dev] [PATCH 3/3] examples/vhost: add new vhost example", "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>,\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": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "X-List-Received-Date": "Wed, 06 Aug 2014 05:32:23 -0000" }, "addressed": null } ]