From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM03-DM3-obe.outbound.protection.outlook.com (mail-dm3nam03on0047.outbound.protection.outlook.com [104.47.41.47]) by dpdk.org (Postfix) with ESMTP id 0DCDF72FC for ; Wed, 14 Sep 2016 02:25:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=SonusNetworks.onmicrosoft.com; s=selector1-sonusnet-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=QsfQxYd2aO9+rh1e9icTjf1jGmhIhC+jXo8ezGa3I4U=; b=DFVnDYlJYjoULSgUsDyfWUjeSlmQK1jILVk2mKaD/xetS/KsMwVFchlElgSOlTcMY+52Msz/jcGc+MXiTRO5XK8EN5eYSisyiQhVcOeUNgqGhUYpXQ3/QkNky5smnnIlncWDErZaUtikF3yCHgBqDJZKwANehAdk+bkfTiAMnwA= Received: from BN3PR03MB1494.namprd03.prod.outlook.com (10.163.35.145) by BN3PR03MB1495.namprd03.prod.outlook.com (10.163.35.146) with Microsoft SMTP Server (version=TLS1_0, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA_P384) id 15.1.619.10; Wed, 14 Sep 2016 00:25:22 +0000 Received: from BN3PR03MB1494.namprd03.prod.outlook.com ([10.163.35.145]) by BN3PR03MB1494.namprd03.prod.outlook.com ([10.163.35.145]) with mapi id 15.01.0619.012; Wed, 14 Sep 2016 00:25:21 +0000 From: "Dey, Souvik" To: "Dey, Souvik" , "Kavanagh, Mark B" , Yuanhan Liu CC: "dev@dpdk.org" , "stephen@networkplumber.org" Thread-Topic: [PATCH v4]net/virtio: add mtu set in virtio Thread-Index: AQHSCL7xEgAVZJyiaUu1GlORUNZIaKBtbHzggANRoICAAHiC0IAADvoAgAAGmlCAAAQsgIAAJdjQgASBloCAAAPRgIACL43g Date: Wed, 14 Sep 2016 00:25:21 +0000 Message-ID: References: <20160907041832.35384-1-sodey@sonusnet.com> <20160909070009.GS23158@yliu-dev.sh.intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=sodey@sonusnet.com; x-originating-ip: [2601:191:8380:14ba:ac96:d8dc:e892:42c9] x-ms-office365-filtering-correlation-id: 5ce98930-294b-47f4-c4be-08d3dc359d4e x-microsoft-exchange-diagnostics: 1; BN3PR03MB1495; 6:HI6WK025cCCUfW2kxrBzoI7ou5iibkPIy0F9dRdAKY8GW26x/1ydB7bmfEfoRcyi0DwgAU8VqXZlG0G0UOnD9PumfQP4CnFcmWuzyRHazdE1EQDvr1mmbfhS/0KnOuqLXXUbPL74sZsB6EoEI8LRiA5IUxUFXYqn2YbroMH8BX4HATQfg2OXGTinxH5r6jxHk3LMziUef756K2dU2Fm/Qa9NSqTJwheCHtanRmhYefjgF1AEdlHUl13ZuT1+UsGox+PiHIhFmGeDlrqEe/08ATv6HOYtdBAc/eMof4vj06Y=; 5:rgWwnU0AFtRpX36szMoSgIicAKrSA8eD0+paF3HzoRtgkDWUmxCfWDEPIsWGipcUK4EIuv8HMOP4CKBcjgvB225Zx1JHqFcloC5Tzp91y68Uw9PHDVCp8CorxNe0NjjMNvlrE4Hc/ueBiRIMLG5QBw==; 24:eRuvMhOI7pE+HmBevwaUwEIfRa8AiQd3oqG9LLdVC9J2En4pfwG7Eiduef4tP/sFP5pgnFp4q+rcVpq9nGOSrcaP5ggFoLP6EZOL/reskC4=; 7:et0Vzr1+y4SUalDPuPEkBg0UM02GNDVYvFlF+YyqkgiHTavIWZLjsxZsoL8heqeU1riLwkZ7v3XbvqFVPhScnNzS+3dqECk0jlPDgU8LVgmWmdb6hOPmHfLi4ot1TIp9g5CnTLnpO72I9aZ40DED6/t9Qvtc4dogwF/c0Qs6ngPgOwkb8l835PC0nE9dQ7AUjJdDQPZhTx3spldl4tlQshe7DDwmgPiJ23FXddJfBuU4iIatDb5VWFqVw9zLHqAt x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BN3PR03MB1495; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(158342451672863)(228905959029699); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040176)(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046); SRVR:BN3PR03MB1495; BCL:0; PCL:0; RULEID:; SRVR:BN3PR03MB1495; x-forefront-prvs: 006546F32A x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(377454003)(189002)(199003)(13464003)(129404003)(54534003)(24454002)(51914003)(7696004)(5002640100001)(33656002)(122556002)(93886004)(189998001)(76576001)(77096005)(68736007)(81156014)(81166006)(8676002)(5660300001)(87936001)(8936002)(9686002)(7846002)(5001770100001)(97736004)(74316002)(19580405001)(3660700001)(586003)(6116002)(4326007)(50986999)(3280700002)(102836003)(54356999)(2906002)(101416001)(7736002)(76176999)(2950100001)(10400500002)(2900100001)(99286002)(106116001)(19580395003)(105586002)(305945005)(3900700001)(92566002)(106356001)(86362001)(21314002)(3826002); DIR:OUT; SFP:1101; SCL:1; SRVR:BN3PR03MB1495; H:BN3PR03MB1494.namprd03.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; received-spf: None (protection.outlook.com: sonusnet.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: sonusnet.com X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Sep 2016 00:25:21.6541 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 29a671dc-ed7e-4a54-b1e5-8da1eb495dc3 X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN3PR03MB1495 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: Wed, 14 Sep 2016 00:25:25 -0000 Hi Mark/Liu, Is there any further comments to the below patch ? Should I submit it as v= 5 of the patch ?=20 -- Regards, Souvik -----Original Message----- From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dey, Souvik Sent: Monday, September 12, 2016 11:12 AM To: Kavanagh, Mark B ; Yuanhan Liu Cc: dev@dpdk.org; stephen@networkplumber.org Subject: Re: [dpdk-dev] [PATCH v4]net/virtio: add mtu set in virtio =09 -----Original Message----- From: Kavanagh, Mark B [mailto:mark.b.kavanagh@intel.com] Sent: Monday, September 12, 2016 10:48 AM To: Dey, Souvik ; Yuanhan Liu Cc: dev@dpdk.org; stephen@networkplumber.org Subject: RE: [PATCH v4]net/virtio: add mtu set in virtio > >Hi Mark/Liu, >=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 Thanks to both of you for being so= patient with a series=20 >of silly errors. I have tried to make it better this time. Also there=20 >were some un-used variable in the function which I have removed. Please=20 >check the new patch with all your comments incorporated. Also along with t= he L2_HDR_LEN and L2_CRC_LEN, I have taken in consideration the VLAN_LEN to= o. > >One doubt though, >"9728 is the largest L2 frame size allowed by the NIC" -- this 9728=20 >size is some constrain due to DPDK as the virtio driver in the kernel=20 >can support till mtu size of 68 to 65535, where as in DPDK pmd we are tryi= ng with 64 to 9728. Yes, I meant the NIC as controlled by a DPDK PMD (I thought this was implic= it, given the context of this discussion). I'll be more explicit in future = mails though. > >drivers/net/virtio/virtio_ethdev.c | 19 +++++++++++++++++++ >1 file changed, 19 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 >@@ -653,12 +653,20 @@ virtio_dev_allmulticast_disable(struct >rte_eth_dev *dev) >=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PMD_INIT_LOG(ERR, "Failed to= disable allmulticast"); } > >+#define VLAN_TAG_LEN=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 4=A0=A0=A0 /* 802.3ac = tag (not DMA'd) */ >+ >+static int=A0 virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) One thing on this function: it doesn't actually set any fields, but rather = just sanitizes 'mtu'. Maybe this is acceptable, since the calling function (i.e. rte_eth_dev_set_= mtu) sets rte_eth_dev->data->mtu? [Dey, Souvik] Yes , only the sanity check for the mtu is required here and= the setting of the call back, else the rte_eth_dev_set_mtu() just returns = without setting the MTU in the rte_eth_dev->data->mtu. >+{ >+=A0=A0=A0=A0=A0=A0 struct rte_eth_dev_info dev_info; >+=A0=A0=A0=A0=A0=A0 uint32_t ether_hdr_len =3D ETHER_HDR_LEN + ETHER_CRC_L= EN +=20 >+VLAN_TAG_LEN; >+=A0=A0=A0=A0=A0=A0 uint32_t frame_size =3D mtu + ether_hdr_len; >+ >+=A0=A0=A0=A0=A0=A0 virtio_dev_info_get(dev, &dev_info); >+ >+=A0=A0=A0=A0=A0=A0 if (mtu < dev_info.min_rx_bufsize || frame_size > >+dev_info.max_rx_pktlen) { It's not clear to me whether 'mtu' in this case should be compared with ETH= ER_MIN_MTU, as per other DPDK drivers, or alternatively whether 'frame_size= ' should be compared with dev_info.min_rx_bufsize. Any thoughts? [Dey, Souvik] I am not sure why virtio min_rx_bufsize is less than ETHER_MI= N_MTU, i think it will be good to have the compare statement as If(frame_s= ize < ETHER_MIN_MTU || frame_size > dev_info.max_rx_pktlen) , then error. W= hat do you suggest ? >+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PMD_INIT_LOG(ERR, "MTU should = be between %d and %d\n", >+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0dev_info.min_rx_bufsize, As above. >+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0 (dev_info.max_rx_pktlen -=20 >+ether_hdr_len)); >+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; >+=A0=A0=A0=A0=A0=A0 } >+ =A0=A0=A0=A0=A0=A0return 0; >+} >@@ -677,7 +685,6 @@ static const struct eth_dev_ops virtio_eth_dev_ops=20 >=3D { >=A0=A0=A0=A0=A0 =A0=A0.allmulticast_enable=A0=A0=A0=A0 =3D virtio_dev_allm= ulticast_enable, >=A0=A0=A0=A0=A0=A0=A0 .allmulticast_disable=A0=A0=A0 =3D virtio_dev_allmul= ticast_disable, >+=A0=A0=A0=A0=A0=A0 .mtu_set=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0 =3D virtio_mtu_set, >=A0=A0=A0=A0=A0=A0=A0 .dev_infos_get=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =3D vir= tio_dev_info_get, >=A0=A0=A0=A0=A0=A0=A0 .stats_get=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =3D virtio_dev_stats_get, >=A0=A0=A0=A0=A0=A0=A0 .xstats_get=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 = =3D virtio_dev_xstats_get, > > >-- >Regards, >Souvik > >-----Original Message----- >From: Kavanagh, Mark B [mailto:mark.b.kavanagh@intel.com] >Sent: Friday, September 9, 2016 11:44 AM >To: Dey, Souvik ; Yuanhan Liu=20 > >Cc: dev@dpdk.org; stephen@networkplumber.org >Subject: RE: [PATCH v4]net/virtio: add mtu set in virtio > >> >>Hi Mark, >> >>Yes I thought I did that change. Sorry once again.. making too many mista= kes. Changed it . >>Thanks. >>The MTU here is L3 MTU.=A0 Setting this will help in reducing the=20 >>fragmented/multi-segmented packets and also in case we want to reduce=20 >>the MTU below 1500, to support VXLAN or GRE tunnel for the packets in=20 >>openstack and cloud >environments. >> >>--- >> 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 >> >>static int virtio_dev_queue_stats_mapping_set( >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 __rte_unused struct rte_eth_dev *eth_dev,= @@ -652,6 >>+653,16 @@ virtio_dev_allmulticast_disable(struct rte_eth_dev *dev) >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0 PMD_INIT_LOG(ERR, "Failed to disable=20 >>allmulticast");=A0 } >> >>+static int >>+virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) { >>+=A0=A0=A0=A0=A0=A0=A0=A0=A0 struct virtio_hw *hw =3D dev->data->dev_priv= ate; >>+=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > >>+VIRTIO_MAX_RX_PKTLEN) { > >If MTU is the L3 MTU as you've indicated, then you need to take account=20 >of L2 overhead before performing the comparison above. >Say the user supplies 'mtu' as 9728 - the corresponding minimum frame=20 >size is L2_HDR_LEN + >9728 + L2_CRC_LEN =3D 9746 bytes, which is larger than the NIC can=20 >accommodate (note that 9728 is the largest L2 frame size allowed by the NI= C, not the largest IP packet size). > >>+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PM= D_INIT_LOG(ERR, "Mtu should be between=20 >>+VIRTIO_MIN_RX_BUFSIZE and >>VIRTIO_MAX_RX_PKTLEN \n"); > >Two things on this statement: >1) in the context of a log message, VIRTIO_XXX_RX_BUFSIZE most likely=20 >means very little, and as such is not helpful. I suggest going with the=20 >format that I included in my earlier mail, which prints the numeric=20 >value of the min and max rx defines >2) MTU is an initialization, and should be printed as such=20 >in a log (i.e. all >caps) > >Hope this helps, >Mark > >>+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 re= turn -EINVAL; >>+=A0=A0=A0=A0=A0=A0=A0=A0=A0 } >>+=A0=A0=A0=A0=A0=A0=A0=A0=A0 return 0; >>+} >>+ >> /* >>=A0 * dev_ops for virtio, bare necessities for basic operation >>=A0 */ >>@@ -664,6 +675,7 @@ static const struct eth_dev_ops virtio_eth_dev_ops=20 >>=3D { >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 .promiscuous_disable=A0=A0=A0=A0 =3D virt= io_dev_promiscuous_disable, >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 .allmulticast_enable=A0=A0=A0=A0 =3D virt= io_dev_allmulticast_enable, >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 .allmulticast_disable=A0=A0=A0 =3D=20 >>virtio_dev_allmulticast_disable, >>+=A0=A0=A0=A0=A0=A0=A0=A0=A0 .mtu_set=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0 =3D virtio_mtu_set, >> >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 .dev_infos_get=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0 =3D virtio_dev_info_get, >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 .stats_get=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0 =3D virtio_dev_stats_get, >>-- >> >>-- >>Regards, >>Souvik >>-----Original Message----- >>From: Kavanagh, Mark B [mailto:mark.b.kavanagh@intel.com] >>Sent: Friday, September 9, 2016 11:05 AM >>To: Dey, Souvik ; Yuanhan Liu=20 >> >>Cc: dev@dpdk.org; stephen@networkplumber.org >>Subject: RE: [PATCH v4]net/virtio: add mtu set in virtio >> >>> >>>Hi Liu, >>> >>>Yes agreed your comment. I will definitely remove the declaration as=20 >>>it is not really required. >>> So the latest patch will look like this . Yes I did rush a bit to=20 >>>submit the patch last will correct my suite. So sending the patch in=20 >>>a reply if we have more comments we can take a look and they=20 >>>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/virtio_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) { >>>+=A0=A0=A0=A0=A0=A0=A0 struct virtio_hw *hw =3D dev->data->dev_private; >>>+=A0=A0=A0=A0=A0=A0=A0 if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > >>>+VIRTIO_MAX_RX_PKTLEN) { >>>+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PMD_INI= T_LOG(ERR, "Mtu should be between 64=20 >>>+and 9728\n"); >> >>Hi Souvik, >> >>Why hardcode the values in the PMD_INIT_LOG? >> >>Why not=A0 do the following: PMD_INIT_LOG(ERR, "MTU should be between %d= =20 >>and %d", >> >=A0=A0=A0=A0=A0=A0 VIRTIO_MIN_RX_BUFSIZE, >>=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=20 >>VIRTIO_MAX >_RX_PKTLEN); >> >>That way, if the values that the macros evaluate to change, the log=20 >>will update correspondingly. >> >>Also, does 'MTU' in this context relate to the L2 or L3 MTU? >> >>>+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return = -EINVAL; >>>+=A0=A0=A0=A0=A0=A0=A0 } >>>+=A0=A0=A0=A0=A0=A0=A0 return 0; >>>+} >>>+ >>> /* >>>=A0 * dev_ops for virtio, bare necessities for basic operation >>>=A0 */ >>>@@ -664,6 +675,7 @@ static const struct eth_dev_ops=20 >>>virtio_eth_dev_ops =3D { >>> =A0=A0=A0=A0=A0=A0=A0=A0 .promiscuous_disable=A0=A0=A0=A0 =3D virtio_de= v_promiscuous_disable, >>> =A0=A0=A0=A0=A0=A0=A0=A0 .allmulticast_enable=A0=A0=A0=A0 =3D virtio_de= v_allmulticast_enable, >>> =A0=A0=A0=A0=A0=A0=A0=A0 .allmulticast_disable=A0=A0=A0 =3D virtio_dev_= allmulticast_disable, >>>+=A0=A0=A0=A0=A0=A0=A0 .mtu_set=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0 =3D virtio_mtu_set, >>> >>> =A0=A0=A0=A0=A0=A0=A0=A0 .dev_infos_get=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 = =3D virtio_dev_info_get, >>> =A0=A0=A0=A0=A0=A0=A0=A0 .stats_get=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0 =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, >>>> =A0=A0=A0=A0=A0=A0 Submitted the version 4 of the patch as you suggest= ed , >>> >>>Your patch is looking much better. But not really, you ignored few of my= comments. >>> >>>> and have removed the Reviewed by line. >>>> I have still kept the function definition as to follow the same=20 >>>> suit as we have done for >>>other eth_dev_ops. >>> >>>That's because most of the method implementions are after the=20 >>>reference, thus the declaration is needed. >>> >>>For your case, I see no good reason to do that. BTW, if you disagree=20 >>>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;=20 >>>> 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=20 >>>> cloud it is expected to >>>have the consistent mtu across the infrastructure that the dhcp=20 >>>server sends 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=20 >>>will be gone after >apply. >>>In another word, we don't like to see those change log in git=20 >>>history, but we'd like to see them while review. >>> >>>>=A0 drivers/net/virtio/virtio_ethdev.c | 12 ++++++++++++ >>>>=A0 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=20 >>>> rte_eth_dev *dev,=A0 static void >>>virtio_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);=20 >>>static void virtio_mac_addr_set(struct rte_eth_dev *dev, >>>>=A0 =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0 struct ether_addr=20 >>>>*mac_addr); >>>> +static int=A0 virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu); >>>> >>>>=A0 static int virtio_dev_queue_stats_mapping_set( >>>>=A0 =A0=A0=A0=A0=A0 __rte_unused struct rte_eth_dev *eth_dev, @@ -652,6= +653,16=20 >>>>@@ >>>virtio_dev_allmulticast_disable(struct rte_eth_dev *dev) >>>>=A0 =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PMD_INIT_= LOG(ERR, "Failed to disable=20 >>>>allmulticast");=A0 } >>>> >>>> +static int >>>> +virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) { >>>> +=A0=A0=A0=A0 struct virtio_hw *hw =3D dev->data->dev_private; >>>> +=A0=A0=A0=A0 if (mtu < VIRTIO_MIN_RX_BUFSIZE || mtu > >>>> +VIRTIO_MAX_RX_PKTLEN) { >>>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 PMD_INIT_LOG(E= RR, "Mtu should be between 64 and=20 >>>> +9728\n"); >>> >>>I still saw those numbers. >>> >>>=A0=A0=A0=A0=A0=A0=A0=A0=A0 --yliu