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 C75EB11D4 for ; Fri, 9 Sep 2016 17:05:18 +0200 (CEST) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga102.jf.intel.com with ESMTP; 09 Sep 2016 08:05:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,305,1470726000"; d="scan'208";a="6852745" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by orsmga004.jf.intel.com with ESMTP; 09 Sep 2016 08:05:06 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.102]) by IRSMSX151.ger.corp.intel.com ([169.254.4.15]) with mapi id 14.03.0248.002; Fri, 9 Sep 2016 16:05:05 +0100 From: "Kavanagh, Mark B" To: "Dey, Souvik" , Yuanhan Liu CC: "dev@dpdk.org" , "stephen@networkplumber.org" Thread-Topic: [PATCH v4]net/virtio: add mtu set in virtio Thread-Index: AQHSCqVo9QrBVjxw+E+LTSeFyfJhrKBxQBXA Date: Fri, 9 Sep 2016 15:05:04 +0000 Message-ID: References: <20160907041832.35384-1-sodey@sonusnet.com> <20160909070009.GS23158@yliu-dev.sh.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTAyOTc1MGMtYzdjOS00ZDcwLTljZmEtM2Q1NGJlOWQyNmYzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InFmZWRaZzFMZmZYRklvWEl0Wm1sdjNJMXFxUjBDbGVcL1RWa0hEa1F1SUs0PSJ9 x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v4]net/virtio: add mtu set in virtio 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: Fri, 09 Sep 2016 15:05:19 -0000 > >Hi Liu, > >Yes agreed your comment. I will definitely remove the declaration as it is= not really >required. > So the latest patch will look like this . Yes I did rush a bit to submit = the patch last will >correct my suite. So sending the patch in a reply if we have more comments= we can take a look >and they re-submit the final reviewed patch. Thanks for the help though. > >--- > drivers/net/virtio/virtio_ethdev.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > >diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virti= o_ethdev.c >index 07d6449..da16ad4 100644 >--- a/drivers/net/virtio/virtio_ethdev.c >+++ b/drivers/net/virtio/virtio_ethdev.c > >+static int >+virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) >+{ >+ struct virtio_hw *hw =3D dev->data->dev_private; >+ if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > VIRTIO_MAX_RX_PKTLEN) { >+ PMD_INIT_LOG(ERR, "Mtu should be between 64 and 9728\n"); Hi Souvik, Why hardcode the values in the PMD_INIT_LOG? Why not do the following: PMD_INIT_LOG(ERR, "MTU should be between %d and = %d",=20 VIRTIO_MIN_RX_BUFSIZE, VIRTIO_MAX_RX_PKTLEN); That way, if the values that the macros evaluate to change, the log will up= date correspondingly. Also, does 'MTU' in this context relate to the L2 or L3 MTU? >+ return -EINVAL; >+ } >+ return 0; >+} >+ > /* > * dev_ops for virtio, bare necessities for basic operation > */ >@@ -664,6 +675,7 @@ static const struct eth_dev_ops virtio_eth_dev_ops =3D= { > .promiscuous_disable =3D virtio_dev_promiscuous_disable, > .allmulticast_enable =3D virtio_dev_allmulticast_enable, > .allmulticast_disable =3D virtio_dev_allmulticast_disable, >+ .mtu_set =3D virtio_mtu_set, > > .dev_infos_get =3D virtio_dev_info_get, > .stats_get =3D virtio_dev_stats_get, >-- > >-- >Regards, >Souvik > >-----Original Message----- >From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] >Sent: Friday, September 9, 2016 3:00 AM >To: Dey, Souvik >Cc: dev@dpdk.org; stephen@networkplumber.org >Subject: Re: [PATCH v4]net/virtio: add mtu set in virtio > >On Wed, Sep 07, 2016 at 04:21:30AM +0000, Dey, Souvik wrote: >> Hi Liu, >> Submitted the version 4 of the patch as you suggested , > >Your patch is looking much better. But not really, you ignored few of my c= omments. > >> and have removed the Reviewed by line. >> I have still kept the function definition as to follow the same suit as = we have done for >other eth_dev_ops. > >That's because most of the method implementions are after the reference, t= hus the declaration >is needed. > >For your case, I see no good reason to do that. BTW, if you disagree with = my comment, you >shoud made a reply, instead of rushing to sending a new version. > >> -- >> Regards, >> Souvik >> >> -----Original Message----- >> From: Dey, Souvik >> Sent: Wednesday, September 7, 2016 12:19 AM >> To: dev@dpdk.org; stephen@networkplumber.org; >> yuanhan.liu@linux.intel.com >> Cc: Dey, Souvik >> Subject: [PATCH v4]net/virtio: add mtu set in virtio >> >> >> Virtio interfaces should also support setting of mtu, as in case of clou= d it is expected to >have the consistent mtu across the infrastructure that the dhcp server sen= ds and not >hardcoded to 1500(default). >> >> Changes in v4: Incorporated review comments. >> Changes in v3: Corrected few style errors as reported by sys-stv. >> Changes in v2: Incorporated review comments. > >DPDK prefers to put the change log to ... >> >> Signed-off-by: Souvik Dey >> >> --- > >... here. > >So that they will be showed in mailing list (for review), but they will be= gone after apply. >In another word, we don't like to see those change log in git history, but= we'd like to see >them while review. > >> drivers/net/virtio/virtio_ethdev.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/net/virtio/virtio_ethdev.c >> b/drivers/net/virtio/virtio_ethdev.c >> index 07d6449..da16ad4 100644 >> --- a/drivers/net/virtio/virtio_ethdev.c >> +++ b/drivers/net/virtio/virtio_ethdev.c >> @@ -92,6 +92,7 @@ static void virtio_mac_addr_add(struct rte_eth_dev *de= v, static void >virtio_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index); static v= oid >virtio_mac_addr_set(struct rte_eth_dev *dev, >> struct ether_addr *mac_addr); >> +static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); >> >> static int virtio_dev_queue_stats_mapping_set( >> __rte_unused struct rte_eth_dev *eth_dev, @@ -652,6 +653,16 @@ >virtio_dev_allmulticast_disable(struct rte_eth_dev *dev) >> PMD_INIT_LOG(ERR, "Failed to disable allmulticast"); } >> >> +static int >> +virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) { >> + struct virtio_hw *hw =3D dev->data->dev_private; >> + if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > VIRTIO_MAX_RX_PKTLEN) { >> + PMD_INIT_LOG(ERR, "Mtu should be between 64 and 9728\n"); > >I still saw those numbers. > > --yliu