From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 0327A6835 for ; Thu, 30 Oct 2014 11:01:22 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 30 Oct 2014 03:04:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,284,1413270000"; d="scan'208";a="613995582" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.220.88]) by fmsmga001.fm.intel.com with SMTP; 30 Oct 2014 03:10:02 -0700 Received: by (sSMTP sendmail emulation); Thu, 30 Oct 2014 10:10:02 +0100 Date: Thu, 30 Oct 2014 10:10:02 +0000 From: Bruce Richardson To: "Ouyang, Changchun" Message-ID: <20141030101002.GC4460@bricha3-MOBL3> References: <1408932572-10343-1-git-send-email-changchun.ouyang@intel.com> <1414381533-30370-1-git-send-email-changchun.ouyang@intel.com> <1414381533-30370-4-git-send-email-changchun.ouyang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and config VMDQ offload register for multicast feature 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: Thu, 30 Oct 2014 10:01:23 -0000 On Thu, Oct 30, 2014 at 12:49:13AM +0000, Ouyang, Changchun wrote: > Hi, > > > -----Original Message----- > > From: Xie, Huawei > > Sent: Thursday, October 30, 2014 7:26 AM > > To: Ouyang, Changchun; dev@dpdk.org > > Subject: RE: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and > > config VMDQ offload register for multicast feature > > > > > > > > > -----Original Message----- > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang > > Changchun > > > Sent: Sunday, October 26, 2014 8:46 PM > > > To: dev@dpdk.org > > > Subject: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and > > > config VMDQ offload register for multicast feature > > > > > > This patch is to let vhost receive and forward multicast and broadcast > > > packets, add promiscuous option into command line; and set VMDQ RX > > mode as: > > > ETH_VMDQ_ACCEPT_BROADCAST|ETH_VMDQ_ACCEPT_MULTICAST if > > promisc mode is > > > on. > > > > > > Signed-off-by: Changchun Ouyang > > > --- > > > examples/vhost/main.c | 25 ++++++++++++++++++++++--- > > > lib/librte_vhost/virtio-net.c | 4 +++- > > > 2 files changed, 25 insertions(+), 4 deletions(-) > > > > > > diff --git a/examples/vhost/main.c b/examples/vhost/main.c index > > > 291128e..c4947f7 100644 > > > --- a/examples/vhost/main.c > > > +++ b/examples/vhost/main.c > > > @@ -161,6 +161,9 @@ > > > /* mask of enabled ports */ > > > static uint32_t enabled_port_mask = 0; > > > > > > +/* Ports set in promiscuous mode off by default. */ > > comment is confusing > Don't think it is confusing, > But I can refine it a bit. > > > +static uint32_t promiscuous_on; > > > + > > > /*Number of switching cores enabled*/ static uint32_t > > > num_switching_cores = 0; > > Don't initialize static variables to zero/NULL > > I don't touch this line in my patch, and initialization to 0 is not an issue here. > > > > > > > @@ -274,6 +277,7 @@ static struct rte_eth_conf vmdq_conf_default = { > > > .enable_default_pool = 0, > > > .default_pool = 0, > > > .nb_pool_maps = 0, > > > + .rx_mode = 0, > > > .pool_map = {{0, 0},}, > > > }, > > > }, > > Same as above, do we need to initialize static var? > Response same as above, no harm to initialize them to 0. With this one, I would actually disagree. With something like this is it harder to read, since you have a whole list of values given here. The reader has to scan through the list of values to check in case any of them are non-zero. If there is no initializer, or just a single-value initializer, e.g. ... vmdq_conf_default = { .default_pool = 0 }, the user just has a single line to look at and can know that the rest of the values will be zero. Furthermore, having the initializers all spelled out like that uses up screen space, which, if the initializers weren't there would be filled instead by code which is meaningful. More meaningful code on screen again makes things easier for the reader, as they have to do less scrolling to find the context they need. It's not a big deal either way, but that's my opinion on the matter. :-) /Bruce