Re: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and config VMDQ offload register for multicast feature

["Ouyang, Changchun" <changchun.ouyang@intel.com>]

Hi ,

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, October 30, 2014 6:10 PM
> To: Ouyang, Changchun
> Cc: Xie, Huawei; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 3/5] vhost: enable promisc mode and
> config VMDQ offload register for multicast feature
> 
> 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 <changchun.ouyang@intel.com>
> > > > ---
> > > >  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. :-)
> 
Seems at least you 2 guys has strong objection on the initializing 0 to a static var.
Ok for updating, and don't want to spend much time in arguing on the tiny stuff. :-)
Pls waiting for my next version update.

Changchun