From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <mark.b.kavanagh@intel.com>
Received: from mga04.intel.com (mga04.intel.com [192.55.52.120])
by dpdk.org (Postfix) with ESMTP id 8001668F2
for <dev@dpdk.org>; Fri, 23 Sep 2016 11:07:47 +0200 (CEST)
Received: from fmsmga005.fm.intel.com ([10.253.24.32])
by fmsmga104.fm.intel.com with ESMTP; 23 Sep 2016 02:07:46 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.30,381,1470726000"; d="scan'208";a="12456884"
Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59])
by fmsmga005.fm.intel.com with ESMTP; 23 Sep 2016 02:07:45 -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;
Fri, 23 Sep 2016 10:07:45 +0100
From: "Kavanagh, Mark B" <mark.b.kavanagh@intel.com>
To: Yuanhan Liu <yuanhan.liu@linux.intel.com>, Stephen Hemminger
<stephen@networkplumber.org>
CC: "Dey, Souvik" <sodey@sonusnet.com>, "dev@dpdk.org" <dev@dpdk.org>
Thread-Topic: [PATCH v6] net/virtio: add set_mtu in virtio
Thread-Index: AQHSFF2laKZJG9++60WAeesbqX0pc6CEhHiAgAAM+ACAABrzgIAB+TgAgAAku2A=
Date: Fri, 23 Sep 2016 09:07:44 +0000
Message-ID: <DC5AD7FA266D86499789B1BCAEC715F85C71C19D@irsmsx105.ger.corp.intel.com>
References: <20160921231147.26820-1-sodey@sonusnet.com>
<20160921162213.4b79d1ce@xeon-e3>
<BN3PR03MB14941D7F017B6EB98BD19298DAC90@BN3PR03MB1494.namprd03.prod.outlook.com>
<20160921184505.584367ef@xeon-e3>
<20160923075320.GY23158@yliu-dev.sh.intel.com>
In-Reply-To: <20160923075320.GY23158@yliu-dev.sh.intel.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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjU4Y2U4YTYtOTkyNy00N2M3LTkwMWEtYWM3ZTZiODRmZDk3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InhFSHpnaHFyeUVGSWs1R2dYQVFsY1YyaTJIRlFtY0hxeTUxK3Q5THZFaTQ9In0=
x-originating-ip: [163.33.239.181]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH v6] 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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Fri, 23 Sep 2016 09:07:49 -0000
>Subject: Re: [PATCH v6] net/virtio: add set_mtu in virtio
>
>On Wed, Sep 21, 2016 at 06:45:05PM -0700, Stephen Hemminger wrote:
>> On Thu, 22 Sep 2016 00:08:38 +0000
>> "Dey, Souvik" <sodey@sonusnet.com> wrote:
>>
>> > Answers inline.
>> >
>> > --
>> > Regards,
>> > Souvik
>> >
>> > -----Original Message-----
>> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>> > Sent: Wednesday, September 21, 2016 7:22 PM
>> > To: Dey, Souvik <sodey@sonusnet.com>
>> > Cc: mark.b.kavanagh@intel.com; yuanhan.liu@linux.intel.com; dev@dpdk.o=
rg
>> > Subject: Re: [PATCH v6] net/virtio: add set_mtu in virtio
>> >
>> > On Wed, 21 Sep 2016 19:11:47 -0400
>> > Dey <sodey@sonusnet.com> wrote:
>> >
>> > >
>> > > +
>> > > +#define VLAN_TAG_SIZE 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 + V=
LAN_TAG_SIZE;
>> > > + uint32_t frame_size =3D mtu + ether_hdr_len;
>> > > +
>> > > + virtio_dev_info_get(dev, &dev_info);
>> > > +
>> > > + if (mtu < ETHER_MIN_MTU || frame_size > dev_info.max_rx_pktl=
en) {
>> > > + PMD_INIT_LOG(ERR, "MTU should be between %d and %d\n=
",
>> > > + ETHER_MIN_MTU,
>> > > + (dev_info.max_rx_pktlen - ether_hdr_=
len));
>> > > + return -EINVAL;
>> > > + }
>> > > + return 0;
>> > > +}
>> >
>> > I am fine with the general idea of this patch but:
>> > 1. Calling virtio_dev_info_get is needlessly wasteful when all you w=
ant
>> > is to access the max packet length. Since max_rx_pktlen is always
>> > VIRTIO_MAX_RX_PKTLEN, please just use that.
>> > [Dey, Souvik] I am using the virtio_dev_info_get as in future can/may =
support the
>max_rx_pktlen as a variable to be set by the application. This will keep =
the changes future
>proof. As we need to support till 65535 instead of 9728 as the linux does.
>>
>> Fine, then just dereference hw->rx_max_pktlen. Driver code can/should re=
ference
>> its own data directly.
>
>Dey, maybe you could just use VIRTIO_MAX_RX_PKTLEN here, like what you
>did in early versions.
>
>> >
>> > 2. Defining VLAN_TAG_SIZE is irrelevant if doing vlan offload.
>> > [Dey, Souvik] vlan offload is not mandatory. Se again still have vlan =
being sent up to
>the application. In that case we need to consider the vlan length in the E=
thernet size.
>>
>> The code needs to handle both vlan offload (or not), correctly. You are =
assuming
>> the worst case here.
>
>I think we are fine here to assume worst case.
>
>> >
>> > 3. Virtio doesn't insert CRC, therefore CRC_LEN is irrelevant
>> > [Dey, Souvik] I am not sure of this. Mark commented earlier to conside=
r this length too.
>Mark what do you suggest ?
>>
>> Actually, the thing that matters is the size of the merge rx buf header,=
not the CRC.
>
>Right.
My comments were based on my experience with DPDK ethdev PMDs - Stephen and=
Yuanhan have much more experience with virtio, so I'd go with their sugges=
tion.
>
> --yliu
>>
>> The patch is still buggy.
>>