From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 8A40B594E for ; Wed, 6 Aug 2014 07:32:22 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 05 Aug 2014 22:29:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.01,810,1400050800"; d="scan'208";a="584039144" Received: from fmsmsx103.amr.corp.intel.com ([10.19.9.34]) by orsmga002.jf.intel.com with ESMTP; 05 Aug 2014 22:34:47 -0700 Received: from fmsmsx157.amr.corp.intel.com (10.18.116.73) by FMSMSX103.amr.corp.intel.com (10.19.9.34) with Microsoft SMTP Server (TLS) id 14.3.123.3; Tue, 5 Aug 2014 22:34:46 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX157.amr.corp.intel.com (10.18.116.73) with Microsoft SMTP Server (TLS) id 14.3.123.3; Tue, 5 Aug 2014 22:34:46 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.252]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.217]) with mapi id 14.03.0195.001; Wed, 6 Aug 2014 13:34:44 +0800 From: "Xie, Huawei" To: Stephen Hemminger 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: References: <1407254286-23972-1-git-send-email-huawei.xie@intel.com> <1407254286-23972-4-git-send-email-huawei.xie@intel.com> <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" 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Aug 2014 05:32:23 -0000 > -----Original Message----- > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Wednesday, August 06, 2014 11:33 AM > To: Xie, Huawei > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 3/3] examples/vhost: add new vhost example >=20 >=20 > > +static const uint16_t external_pkt_default_vlan_tag =3D 2000; > > +const uint16_t vlan_tags[] =3D { > > + 1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007, > > + 1008, 1009, 1010, 1011, 1012, 1013, 1014, 1015, > > + 1016, 1017, 1018, 1019, 1020, 1021, 1022, 1023, > > + 1024, 1025, 1026, 1027, 1028, 1029, 1030, 1031, > > + 1032, 1033, 1034, 1035, 1036, 1037, 1038, 1039, > > + 1040, 1041, 1042, 1043, 1044, 1045, 1046, 1047, > > + 1048, 1049, 1050, 1051, 1052, 1053, 1054, 1055, > > + 1056, 1057, 1058, 1059, 1060, 1061, 1062, 1063, > > +}; >=20 > Why pre-compute table if it is just: vlan_tag =3D n + 1000? This 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 us= ing vhost library, so I prefer subsequent patches to fix those kind of prob= lems. >=20 >=20 > > + > > +/* Per-device statistics struct */ > > +struct device_statistics { > > + uint64_t tx_total; > > + rte_atomic64_t rx_total_atomic; > > + uint64_t rx_total; > > + uint64_t tx; > > + rte_atomic64_t rx_atomic; > > + uint64_t rx; > > +} __rte_cache_aligned; > > +struct device_statistics dev_statistics[MAX_DEVICES]; >=20 > Doing per-core statistics would be faster than using atomic's > which have implicit lock. For software vm2vm mode, there is contention as two cores could enqueue the= same virtio ring concurrently. >=20 > > +/* > > + * Builds up the correct configuration for VMDQ VLAN pool map > > + * according to the pool & queue limits. > > + */ > > +static inline int > > +get_eth_conf(struct rte_eth_conf *eth_conf, uint32_t num_devices) > > +{ > > + struct rte_eth_vmdq_rx_conf conf; > > + unsigned i; > > + > > + memset(&conf, 0, sizeof(conf)); > > + conf.nb_queue_pools =3D (enum rte_eth_nb_pools)num_devices; > > + conf.nb_pool_maps =3D num_devices; > > + conf.enable_loop_back =3D > > + > vmdq_conf_default.rx_adv_conf.vmdq_rx_conf.enable_loop_back; > > + > > + for (i =3D 0; i < conf.nb_pool_maps; i++) { > > + conf.pool_map[i].vlan_id =3D vlan_tags[i]; > > + conf.pool_map[i].pools =3D (1UL << i); > > + } > > + > > + (void)(rte_memcpy(eth_conf, &vmdq_conf_default, sizeof(*eth_conf))); > > + (void)(rte_memcpy(ð_conf->rx_adv_conf.vmdq_rx_conf, &conf, > > + sizeof(eth_conf->rx_adv_conf.vmdq_rx_conf))); > > + return 0; > > +} >=20 > If function always returns 0 just make it void. > No point in making non-fastpath code inline. > The cast to (void) is unnecessary and just makes things ugly. > Why not use structure assignment rather than memcpy() which is not type s= afe? Agree. As stated above, I prefer subsequent patches to fix those issues inh= erited from old example. How do you think?