From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM03-CO1-obe.outbound.protection.outlook.com (mail-co1nam03on0049.outbound.protection.outlook.com [104.47.40.49]) by dpdk.org (Postfix) with ESMTP id A66F9968 for ; Wed, 14 Sep 2016 23:12:44 +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=3fiYUiWpY7ZS96c2f/x8u/QudeNzmjPM68tMBrvENYM=; b=ZvM76vxdZw6PbkMsF1ZmbEuq+VEj3M7LU548aR1INjIDNNAgn2+uOdI4TCbO8ZEiQlrZk/kELsm2cIDh9/5/XkXju3bO7OKZl/pOJEskURfIJgSUhIbl4mMQDsPcn2skQymbphD/JlHm4mPO5rLR9sSub0lL7PMT53t+KXedpVE= 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; Wed, 14 Sep 2016 21:12:39 +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 21:12:39 +0000 From: "Dey, Souvik" To: "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: AQHSCL7xEgAVZJyiaUu1GlORUNZIaKBtbHzggANRoICAAHiC0IAADvoAgAAGmlCAAAQsgIAAJdjQgASBloCAAAPRgIAC9kuAgACVpVA= Date: Wed, 14 Sep 2016 21:12:38 +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: [208.45.178.4] x-ms-office365-filtering-correlation-id: 32d044d3-86f8-4666-cbdd-08d3dce3dc02 x-microsoft-exchange-diagnostics: 1; BN3PR03MB1493; 6:ZJvkIZE9NSzzA4iw6YSZbcYE3ImFfSm8anfXSHN08vz8P2atkAYoF7qHnEcvZUBayZkE3WWR6Fs0GEvrJOx94g4SZ+KDiy/h/DXFjwtv7HkWxHAyB7rLbeuQ1/dqqM/iyDf+yAtmpzlIdO8if/LuOz0DMWq69Mp/K4icWbPnq6uHKwRnJqa1xIhaQwKS63dGLmZ36JfnQH5JFlS79ARBUxN2kN2RE+hSQWO+NY/LlupwkyBh6ef+1XGRqmYqGzp/ms2F6LkJ5ki2zI3wfJ1sNum9bSd1NWp9wZE1Kfy87Pw=; 5:+PvXB/rM7fZ9Krwp0i0JqOt+dRXQg9BYAVxEcQaz9PxrdNA63tefnx2kAf9/N2RY0foBccEeBhacHx9DEr/LMVBaH+r42k9tqaR5tB3E0MfRbfsTD5UEe9p7OX5q39Mdce5usnUGbiOneMG3NUBbHw==; 24:aZ8RJFl5xxowamRoVKlaNS359GhEzFJ2AgGoqSHSMZskLOGraspcvmb71G2tXH8iU4hsyV0rNK4Fi03BF6rqLm2aEOq8YVsXUwuefogFdvQ=; 7:UltzEte9DPlhD1rfKiIa/OLP3Pyj2TNLiyudtxyaOJy1ThG8/rL/JuwlsfncLMfSHFkk+6AQvijLNVhi94eernpTOIveUvn7LYHejsiuGkWskDUvW6DeM7QIuvQ2/1X49QS6beZVLflvAxultJnCMct8fzt4u/gw1uBpSBC8H3PEXNwzufCkKqKV99LvA8u1GE4gaSwIM5sL7VBvOihoSoZgzElQVUIiORt8rj9BgUAOrROQBPdGBkZzYpnyxa8d 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)(10201501046)(3002001); SRVR:BN3PR03MB1493; BCL:0; PCL:0; RULEID:; SRVR:BN3PR03MB1493; x-forefront-prvs: 006546F32A x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(51694002)(51914003)(24454002)(189002)(377454003)(478694002)(54534003)(13464003)(129404003)(199003)(6116002)(92566002)(2950100001)(19580395003)(189998001)(101416001)(19580405001)(50986999)(54356999)(3900700001)(5660300001)(86362001)(76176999)(77096005)(7846002)(97736004)(2906002)(7736002)(10400500002)(5001770100001)(74316002)(305945005)(11100500001)(4326007)(5002640100001)(122556002)(87936001)(7696004)(9686002)(8936002)(33656002)(105586002)(3660700001)(76576001)(99286002)(3280700002)(3846002)(102836003)(586003)(2900100001)(81166006)(81156014)(8676002)(106116001)(66066001)(68736007)(93886004)(106356001)(21314002); DIR:OUT; SFP:1101; SCL:1; SRVR:BN3PR03MB1493; 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 21:12:38.9514 (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: Wed, 14 Sep 2016 21:12:45 -0000 Hi Mark/Liu, I know this might be a redundant question, but should I put your names in = the Reviewed-by section in v5 ?=20 -- Regards, Souvik -----Original Message----- From: Kavanagh, Mark B [mailto:mark.b.kavanagh@intel.com]=20 Sent: Wednesday, September 14, 2016 8:16 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 s= o 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.=20 >>Please check the new patch with all your comments incorporated. Also=20 >>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=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=20 >>trying with 64 to >9728. > >Yes, I meant the NIC as controlled by a DPDK PMD (I thought this was=20 >implicit, 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 t= o 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=20 >rather just sanitizes 'mtu'. >Maybe this is acceptable, since the calling function (i.e.=20 >rte_eth_dev_set_mtu) sets rte_eth_dev->data->mtu? >[Dey, Souvik] Yes , only the sanity check for the mtu is required here=20 >and the setting of the call back, else the rte_eth_dev_set_mtu() just=20 >returns without setting the MTU in the rte_eth_dev->data->mtu. Hi Souvik, apologies for the delayed response. That's what I thought, just wanted to verify that no additional structures = should be updated here. > >>+{ >>+=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_= LEN +=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=20 >ETHER_MIN_MTU, as per other DPDK drivers, or alternatively whether=20 >'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=20 >ETHER_MIN_MTU, i think it will be good to have the compare statement=20 >as If(frame_size < ETHER_MIN_MTU || frame_size >=20 >dev_info.max_rx_pktlen) , then error. What do you suggest ? Again, this all depends on what 'mtu' means in this context. Since you mentioned previously that it relates to the packet (i.e. L3) leng= th, and not the frame (i.e. L2) length, I would suggest that the comparison= should be: if (mtu < ETHER_MIN_MTU || frame_size > dev_info.max_rx_pktlen) Yuanhan, any thoughts on this? > >>+=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_all= multicast_enable, >>=A0=A0=A0=A0=A0=A0=A0 .allmulticast_disable=A0=A0=A0 =3D virtio_dev_allmu= lticast_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 vi= rtio_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 mist= akes. 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_pri= vate; >>>+=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=20 >>account 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=20 >>the NIC, 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 P= MD_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=20 >>the format that I included in my earlier mail, which prints the=20 >>numeric 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 r= eturn -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=20 >>>virtio_eth_dev_ops =3D { >>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 .promiscuous_disable=A0=A0=A0=A0 =3D=20 >>>virtio_dev_promiscuous_disable, >>> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 .allmulticast_enable=A0=A0=A0=A0 =3D=20 >>>virtio_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_IN= IT_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=20 >>>%d and %d", >>> >>=A0=A0=A0=A0=A0=A0 VIRTIO_MIN_RX_BUFSIZE, >>> >>>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_d= ev_promiscuous_disable, >>>> =A0=A0=A0=A0=A0=A0=A0=A0 .allmulticast_enable=A0=A0=A0=A0 =3D virtio_d= ev_allmulticast_enable, >>>> =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 .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 sugges= ted , >>>> >>>>Your patch is looking much better. But not really, you ignored few of m= y 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=20 >>>>> of 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=20 >>>>>ether_addr *mac_addr); >>>>> +static int=A0 virtio_mtu_set(struct rte_eth_dev *dev, uint16_t=20 >>>>> +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(= ERR, "Mtu should be between 64=20 >>>>> +and 9728\n"); >>>> >>>>I still saw those numbers. >>>> >>>>=A0=A0=A0=A0=A0=A0=A0=A0=A0 --yliu