From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id CC9942A58 for ; Wed, 21 Sep 2016 17:22:58 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga103.jf.intel.com with ESMTP; 21 Sep 2016 08:22:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,374,1470726000"; d="scan'208";a="882326404" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by orsmga003.jf.intel.com with ESMTP; 21 Sep 2016 08:22:57 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.196]) by IRSMSX151.ger.corp.intel.com ([169.254.4.226]) with mapi id 14.03.0248.002; Wed, 21 Sep 2016 16:22:55 +0100 From: "Kavanagh, Mark B" To: souvikdey33 , "dev@dpdk.org" , "stephen@networkplumber.org" , "yuanhan.liu@linux.intel.com" Thread-Topic: [PATCH v5]net/virtio: add mtu set in virtio Thread-Index: AQHSED3F7vOv0Ed4GEaq9ep0cUN3kKCDrpHg Date: Wed, 21 Sep 2016 15:22:55 +0000 Message-ID: References: <20160916171357.69640-1-sodey@sonusnet.com> In-Reply-To: <20160916171357.69640-1-sodey@sonusnet.com> 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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNzBjY2IyMzctNWI5Ny00MWEwLWEzYTEtOTcxYWNkMDU2MGM4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InYxNjFQQU9LSWtjc0dpeHg1MGR0Wmd5TUs3RnZwZXFZTzlhS0VBTXJoR2c9In0= x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v5]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, 21 Sep 2016 15:22:59 -0000 >=09 > Hi Souvik, There are some very basic errors in this patch, particularly with respect t= o format. Review comments are inline - please address same and resubmit the patch. I = also recommend running $DPDK_DIR/utilities/checkpatch.py on any future subm= issions. Thanks, Mark > As a general comment, please capitalize 'MTU' throughout the commit message= . >Virtio interfaces should also support setting of mtu, as in case of cloud 'also' is redundant in the above sentence - please remove it. =20 >it is expected to have the consistent mtu across the infrastructure that >the dhcp server sends and not hardcoded to 1500(default). Try to make the problem statement here more general, before going into spec= ifics. i.e. something along the lines of=20 "Virtio interfaces do not currently allow the user to specify a particular= Maximum Transmission Unit (MTU). Consequently, the MTU of Virtio interfaces is typically set to the E= thernet default value of 1500. This is problematic in the case of cloud deployments, in which a spe= cific (and potentially non-standard) MTU needs to be set by a DHCP server, = and honored by all interfaces across the traffic path. ...., etc., etc." = =20 >In case when GRE/VXLAN tunneling is used, it adds overheads to the total Please be specific in your description - what is 'it' in the above context? >size of 1518, and in that cases the DHCP server corrects the L3 MTU to 145= 4 >to take care of the overhead. BUt since virtio interfaces was not having t= he Should be lowercase 'u' in 'But'=20 >mtu set that mtu sent by dhcp was ignored and teh instance will still send Typo - 'teh' >packets with 1500 mtu which afetr encapsulation will become more than 1518 Typo - 'afetr' >and eventually gets dropped in the infrastructure. This issue can be solve= d >by honoring the mtu 1454 sent by dhcp server, which this below patch will >take care. Explain how the patch resolves the issue (i.e. by adding an additional 'set= _mtu' function to the Virtio driver, which can be leveraged by the dhcp ser= ver/controller). > > >Changes in v5: Incorporated review comments. It is implicit that a new patch version incorporates review comments. To make things easier for the reviewer, please provide concise descriptions= of the changes made/review comments addressed. For example: v5: - Fix log message for out-of-bounds MTU parameter in virtio_mtu_set - Calculate frame size, based on 'mtu' parameter, for upper bounds check in= virtio_mtu_set - .... - etc., etc.=20 >Changes in v4: Incorporated review comments. >Changes in v3: Corrected few style errors as reported by sys-stv. >Changes in v2: Incorporated review comments. > >Signed-off-by: Souvik Dey > >--- Version history should not be part of the commit message - please move it t= o below this dashed line. > 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/virti= o_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) > PMD_INIT_LOG(ERR, "Failed to disable allmulticast"); > } > >+#define VLAN_TAG_SIZE 4 /* 802.3ac tag (not DMA'd) */ >+ >+static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) Additional whitespace between 'int' and 'virtio_mtu_set' - please remove. >+{ >+ struct rte_eth_dev_info dev_info; >+ uint32_t ether_hdr_len =3D ETHER_HDR_LEN + ETHER_CRC_LEN + VLAN_TA= G_SIZE; >+ uint32_t frame_size =3D mtu + ether_hdr_len; >+ >+ virtio_dev_info_get(dev, &dev_info); >+ >+ if (frame_size < ETHER_MIN_MTU || frame_size > dev_info.max_rx_pkt= len) { >+ PMD_INIT_LOG(ERR, "MTU should be between %d and %d\n", >+ (ETHER_MIN_MTU - ether_hdr_len), >+ (dev_info.max_rx_pktlen - ether_hdr_len)); >+ return -EINVAL; >+ } >+ return 0; >+} >@@ -677,7 +685,6 @@ static const struct eth_dev_ops virtio_eth_dev_ops =3D= { > .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, > .xstats_get =3D virtio_dev_xstats_get, >-- >2.9.3.windows.1