From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM03-BY2-obe.outbound.protection.outlook.com (mail-by2nam03on0077.outbound.protection.outlook.com [104.47.42.77]) by dpdk.org (Postfix) with ESMTP id BFABE532C for ; Mon, 12 Sep 2016 16:25:20 +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=VCSIS97CEZYzOU41YuBExTT1+Y6BkiUbf3VSntDDF4M=; b=hMJDjo+y0h+ZI2vAuWALgtRnTLq8Hye+YcqMCbldPxR2cxwxWH2tLtiOBjV9miBXm1T8KA4mUDliPQ0PN3f8ExDqM4AtGPYQuvmLy1bkCkpKC2bjnGeZOWauryqTA3U2+Pa3eHRgtS3/Ba7G8IXFoBqI6/zn2Ak717ewNeT81Oo= Received: from BN3PR03MB1494.namprd03.prod.outlook.com (10.163.35.145) by BN3PR03MB1493.namprd03.prod.outlook.com (10.163.35.144) with Microsoft SMTP Server (version=TLS1_0, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA_P384) id 15.1.599.9; Mon, 12 Sep 2016 14:25:18 +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.011; Mon, 12 Sep 2016 14:25:18 +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: AQHSCL7xEgAVZJyiaUu1GlORUNZIaKBtbHzggANRoICAAHiC0IAADvoAgAAGmlCAAAQsgIAAJdjQgAR7BmA= Date: Mon, 12 Sep 2016 14:25:18 +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: sonusnet.com; dkim=none (message not signed) header.d=none;sonusnet.com; dmarc=none action=none header.from=sonusnet.com; x-originating-ip: [208.45.178.4] x-ms-office365-filtering-correlation-id: f590ec3c-c83a-46f2-7f5b-08d3db189f36 x-microsoft-exchange-diagnostics: 1; BN3PR03MB1493; 6:yTGKGXEMCZD6jyf8EHXlWFf6FOakVCDt7PAj9JmQSonryvNXKY2tqHwJwqUClaSLS3aQQgBan/yUwZQV7UwOSAyA9Ci2NeKyT97IKyphT3kcHSA+RysDkg0CigZ//HAuyjHbTDFsuK0DmCKZp+6xiRdu/8bS/A8FPKbBKcWFpsJ7cCcM/eVruvYGBM/AsllItY1SKFL7vwqmHG2t9fmxTkIp/7jn5m0z1ajsNfAQ66w17ZM8i47yZOLZtq/3VIMjRYsmz8vJKpVQjfXgIhPY0Ce/gOWJwqH6P6RiZSK+VKk=; 5:+UG8D+vueGkffq21H19qB5LvPNISaL1cBEVBcZdIAHyK585dCSxq767D5xbQS4oopsf2GQfESVJb5QrmqDwXwDyzIGvxuaMj7eB71CqnReiifJT6nnCCcm9QhgdCVavo5jx5qsYsO5cidPG+S3WhAg==; 24:tEQr345lnHT0qOgy4zlt55drgBYQwLLZcetbx7n2k06/49Z7MDbSAE4IhMr5SnbF/zYpU20/RoU1szswvdBENU8nK38i6Ds7DCP4/zwYPsI=; 7:kWZsQoYETyh83YdxN/YPMJin4A9cqQqEojSjHNjWpeTEXVlW97LTt1PN36ag5X0AiEWoXvI7bepTNRrzB4RsYlMGqtC3/vaL7E2Se4I7sjAMrAzw6/x4ApkQ4QmGgW/UMOH8SY4P0dGZUDVAPPeSuO8D+/eFvcNd7bGAKayjHMSsR6h/j74mO+e3IjbfumjULicWhia2cojBTgxIaBLUCKKppBfBiJWEsKhg5955kpygAf1l1fmqq/mEfwtTMsss x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BN3PR03MB1493; 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:BN3PR03MB1493; BCL:0; PCL:0; RULEID:; SRVR:BN3PR03MB1493; x-forefront-prvs: 006339698F x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(13464003)(53754006)(54534003)(129404003)(377454003)(51914003)(24454002)(8936002)(2950100001)(2900100001)(3280700002)(3660700001)(9686002)(586003)(3846002)(102836003)(76576001)(33656002)(6116002)(99286002)(93886004)(81166006)(8676002)(106116001)(66066001)(5660300001)(189998001)(77096005)(50986999)(19580405001)(19580395003)(92566002)(4001520100001)(54356999)(76176999)(3900700001)(87386001)(5002640100001)(11100500001)(305945005)(122556002)(7696004)(74316002)(4326007)(7736002)(86362001)(2906002)(10400500002)(7846002)(87936001)(5001770100001)(5001760100003)(21314002); DIR:OUT; SFP:1101; SCL:1; SRVR:BN3PR03MB1493; H:BN3PR03MB1494.namprd03.prod.outlook.com; FPR:; SPF:None; LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: sonusnet.com X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Sep 2016 14:25:18.1855 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 29a671dc-ed7e-4a54-b1e5-8da1eb495dc3 X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN3PR03MB1493 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: Mon, 12 Sep 2016 14:25:21 -0000 Hi All, Any further comments or updates to be made in the below patch else I will= go ahead a submit the v5 for the same. -- Regards, Souvik -----Original Message----- From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dey, Souvik Sent: Friday, September 9, 2016 2:16 PM 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 Hi Mark/Liu, Thanks to both of you for being so patient with a series of s= illy errors. I have tried to make it better this time. Also there were some= un-used variable in the function which I have removed. Please check the ne= w patch with all your comments incorporated. Also along with the L2_HDR_LEN= and L2_CRC_LEN, I have taken in consideration the VLAN_LEN too. One doubt though, "9728 is the largest L2 frame size allowed by the NIC" -- this 9728 size is= some constrain due to DPDK as the virtio driver in the kernel can support = till mtu size of 68 to 65535, where as in DPDK pmd we are trying with 64 to= 9728. 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 *d= ev) 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 rte_eth_dev_info dev_info; + uint32_t ether_hdr_len =3D ETHER_HDR_LEN + ETHER_CRC_LEN + VLAN_TAG= _LEN; + uint32_t frame_size =3D mtu + ether_hdr_len; + + virtio_dev_info_get(dev, &dev_info); + + if (mtu < dev_info.min_rx_bufsize || frame_size > dev_info.max_rx_p= ktlen) { + PMD_INIT_LOG(ERR, "MTU should be between %d and %d\n", + dev_info.min_rx_bufsize, + (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, -- 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 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 mistak= es. Changed it . >Thanks. >The MTU here is L3 MTU. Setting this will help in reducing the >fragmented/multi-segmented packets and also in case we want to reduce >the MTU below 1500, to support VXLAN or GRE tunnel for the packets in open= stack 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( > __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 allmultica= st"); } > >+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) = { If MTU is the L3 MTU as you've indicated, then you need to take account of = L2 overhead before performing the comparison above. Say the user supplies 'mtu' as 9728 - the corresponding minimum frame size = is L2_HDR_LEN + 9728 + L2_CRC_LEN =3D 9746 bytes, which is larger than the = NIC can accommodate (note that 9728 is the largest L2 frame size allowed by= the NIC, not the largest IP packet size). >+ PMD_INIT_LOG(ERR, "Mtu should be between VIRTIO_M= IN_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 means= very little, and as such is not helpful. I suggest going with the format t= hat I included in my earlier mail, which prints the numeric value of the mi= n and max rx defines 2) MTU is an initialization, and should be printed as such in a = log (i.e. all caps) Hope this helps, Mark >+ 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: Kavanagh, Mark B [mailto:mark.b.kavanagh@intel.com] >Sent: Friday, September 9, 2016 11:05 AM >To: Dey, Souvik >; Yuanhan L= iu >> >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 >>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/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) { >>+ 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 97= 28\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", > = VIRTIO_MIN_RX_BUFSIZE, > = VIRTIO_MAX_RX_PKTLEN); > >That way, if the values that the macros evaluate to change, the log >will update 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 = comments. >> >>> 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, thus 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 >>> cloud it is expected to >>have the consistent mtu across the infrastructure that the dhcp 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 will b= e 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 >>> *dev, static void >>virtio_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index); >>static void virtio_mac_addr_set(struct rte_eth_dev *dev, >>> struct ether_addr *ma= c_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 972= 8\n"); >> >>I still saw those numbers. >> >> --yliu