From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 6B4555589 for ; Thu, 6 Oct 2016 15:59:03 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga105.jf.intel.com with ESMTP; 06 Oct 2016 06:59:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,454,1473145200"; d="scan'208";a="1050288152" Received: from irsmsx154.ger.corp.intel.com ([163.33.192.96]) by fmsmga001.fm.intel.com with ESMTP; 06 Oct 2016 06:59:01 -0700 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by IRSMSX154.ger.corp.intel.com (163.33.192.96) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 6 Oct 2016 14:59:00 +0100 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.196]) by irsmsx112.ger.corp.intel.com ([169.254.1.170]) with mapi id 14.03.0248.002; Thu, 6 Oct 2016 14:59:00 +0100 From: "Kavanagh, Mark B" To: "Dey, Souvik" , "yuanhan.liu@linux.intel.com" , "stephen@networkplumber.org" CC: "dev@dpdk.org" Thread-Topic: [PATCH v7] net/virtio: add set_mtu in virtio Thread-Index: AQHSGpCJDQZFRvYfBk2V8UZwgCllbqCTpQrAgAVQiPCAAIwv4IAAW0oAgAGLSACAABJrAA== Date: Thu, 6 Oct 2016 13:58:59 +0000 Message-ID: References: <20160922135643.37636-1-sodey@sonusnet.com> <20160929203130.58712-1-sodey@sonusnet.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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOGZkMmMwYzktYTUyZS00MWJjLTk0NTAtMmY3NTk5MGI2ZTJhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlhnWHpsa0xTbTh6NllhZEprUklHd1wvSTZlSmhiRE9RckpKNk1vZUZuRjBnPSJ9 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 v7] net/virtio: add set_mtu 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: Thu, 06 Oct 2016 13:59:04 -0000 > >Hi Stephen/Liu, > Any other comments or suggestions for this. If the below patch look= s fine then please >do suggest the next steps . Hi Souvik, Just to let you know that Yuanhan is out of office on account of public hol= idays in PRC. AFAIA he should be back online from next week. Thanks, Mark > >-- >Regards, >Souvik > >-----Original Message----- >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dey, Souvik >Sent: Wednesday, October 5, 2016 10:05 AM >To: Kavanagh, Mark B ; yuanhan.liu@linux.intel.= com; >stephen@networkplumber.org >Cc: dev@dpdk.org >Subject: Re: [dpdk-dev] [PATCH v7] net/virtio: add set_mtu in virtio > >Yes Mark, I have modified the patch with the below comments. > >drivers/net/virtio/virtio_ethdev.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > >diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virti= o_ethdev.c >index 423c597..1dbfea6 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_LEN 4 /* 802.3ac tag (not DMA'd) */ >+ >+static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) { >+ struct virtio_hw *hw =3D dev->data->dev_private; >+ uint32_t ether_hdr_len =3D ETHER_HDR_LEN + VLAN_TAG_LEN + >+ hw->vtnet_hdr_size; >+ uint32_t frame_size =3D mtu + ether_hdr_len; >+ >+ if (mtu < ETHER_MIN_MTU || frame_size > VIRTIO_MAX_RX_PKTLEN) { >+ PMD_INIT_LOG(ERR, "MTU should be between %d and %d\n", >+ ETHER_MIN_MTU, (VIRTIO_MAX_RX_PKTLEN - ether_hdr_len)); >+ return -EINVAL; >+ } >+ return 0; >+} > >Let mem know if this looks good or we have few more comments. > >-- >Regards, >Souvik > >-----Original Message----- >From: Kavanagh, Mark B [mailto:mark.b.kavanagh@intel.com] >Sent: Wednesday, October 5, 2016 4:16 AM >To: Dey, Souvik ; yuanhan.liu@linux.intel.com; stephen= @networkplumber.org >Cc: dev@dpdk.org >Subject: RE: [PATCH v7] net/virtio: add set_mtu in virtio > >>Hi All, >> Is there any further comments or modifications required for this >>patch, or what next steps do you guys suggest here ? > >Hi Souvik, > >Some minor comments inline. > >Thanks, >Mark > >> >>-- >>Regards, >>Souvik >> >>-----Original Message----- >>From: Dey, Souvik >>Sent: Saturday, October 1, 2016 10:09 AM >>To: mark.b.kavanagh@intel.com; yuanhan.liu@linux.intel.com; >>stephen@networkplumber.org; dev@dpdk.org >>Subject: RE: [PATCH v7] net/virtio: add set_mtu in virtio >> >>Hi Liu/Stephen/Mark, >> >> I have submitted Version 7 of this patch. Do let me know if this looks p= roper. >> >>-- >>Regards, >>Souvik >> >>-----Original Message----- >>From: Dey, Souvik >>Sent: Thursday, September 29, 2016 4:32 PM >>To: mark.b.kavanagh@intel.com; yuanhan.liu@linux.intel.com; >>stephen@networkplumber.org; dev@dpdk.org >>Cc: Dey, Souvik >>Subject: [PATCH v7] net/virtio: add set_mtu in virtio >> >> >>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 Ethernet default value of 1500. >>This is problematic in the case of cloud deployments, in which a >>specific (and potentially non-standard) MTU needs to be set by a DHCP >>server, which needs to be honored by all interfaces across the traffic >>path.To achieve this Virtio interfaces should support setting of MTU. >>In case when GRE/VXLAN tunneling is used for internal communication, >>there will be an overhead added by the infrastructure in the packet >>over and above the ETHER MTU of 1518. So to take care of this overhead >>in these cases the DHCP server corrects the L3 MTU to 1454. But since >>virtio interfaces was not having the MTU set functionality that MTU >>sent by the DHCP server was ignored and the instance will still send >>packets with 1500 MTU which after encapsulation will become more than >>1518 and eventually gets dropped in the infrastructure. >>By adding an additional 'set_mtu' function to the Virtio driver, we can >>honor the MTU sent by the DHCP server. The dhcp server/controller can >>then leverage this 'set_mtu' functionality to resolve the above >>mentioned issue of packets getting dropped due to incorrect size. >> >> >>Signed-off-by: Souvik Dey >> >>--- >>Changes in v7: >>- Replaced the CRC_LEN with the merge rx buf header length. >>- Changed the frame_len max validation to VIRTIO_MAX_RX_PKTLEN. >>Changes in v6: >>- Description of change corrected >>- Corrected the identations >>- Corrected the subject line too >>- The From line was also not correct >>- Re-submitting as the below patch was not proper Changes in v5: >>- Fix log message for out-of-bounds MTU parameter in virtio_mtu_set >>- Calculate frame size, based on 'mtu' parameter >>- Corrected the upper bound and lower bound checks in virtio_mtu_set >>Changes in v4: Incorporated review comments. >>Changes in v3: Corrected few style errors as reported by sys-stv. >>Changes in v2: Incorporated review comments. >> >> drivers/net/virtio/virtio_ethdev.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >>diff --git a/drivers/net/virtio/virtio_ethdev.c >>b/drivers/net/virtio/virtio_ethdev.c >>index 423c597..1dbfea6 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_LEN 4 /* 802.3ac tag (not DMA'd) */ > >There should be a blank line between the #define and the function prototyp= e beneath. > >>+static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) { >>+ struct virtio_hw *hw =3D dev->data->dev_private; >>+ uint32_t ether_hdr_len =3D ETHER_HDR_LEN + VLAN_TAG_LEN + >>+ hw->vtnet_hdr_size; > >I'll rely on Stephen and Yuanhan's judgment for this. > >>+ uint32_t frame_size =3D mtu + ether_hdr_len; >>+ >>+ if (mtu < ETHER_MIN_MTU || frame_size > VIRTIO_MAX_RX_PKTLEN) { >>+ PMD_INIT_LOG(ERR, "MTU should be between %d and %d\n", >>+ ETHER_MIN_MTU, VIRTIO_MAX_RX_PKTLEN); > >Shouldn't last format print parameter should be (VIRTIO_MAX_RX_PKTLEN - et= her_hdr_len)? >i.e PMD_INIT_LOG(ERR, "MTU should........%d\n", > ETHER_MIN_MTU, (VIRTIO_MAX_RX_PKTLEN - ether_hdr_len)); > >>+ return -EINVAL; >>+ } >>+ return 0; >>+} >> >> /* >> * dev_ops for virtio, bare necessities for basic operation >> */ >>@@ -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