From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f41.google.com (mail-pa0-f41.google.com [209.85.220.41]) by dpdk.org (Postfix) with ESMTP id 8D1B12BB5 for ; Tue, 27 Sep 2016 20:56:23 +0200 (CEST) Received: by mail-pa0-f41.google.com with SMTP id oz2so8224955pac.2 for ; Tue, 27 Sep 2016 11:56:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=QpRyhV0ioNwEBbWGEQ9+MPgX6s7F/EgYBuB/xL1fFb8=; b=BG4Yr2pvtsSHBvSiC/wbIoqUzUUC68uVB7jpJ5bjr+dANJaByiNOiKAZnZRGJIZfT1 d/P2iuj3Nvx6frwA92mpdbL5mpaFOOLw/jklz+LB7txRuTGl3LzAfGUnucVDLGhUj5ND Mnj+Jny5FpQv3SXaszxWUQH+ncFbqClOBKTREBhAiJSEGN8In3HjcDszIfeA7DMkyBXc 9ue0i0XFeO0dXYX7pU8VzmBWBiJ8ZiSOqe9BojKMI5SWFEZ7l4EKw9xHheo+fP/BoWvX WL5S28PvOk50Zgh2sVmSHAQo7tij+cmpUaEbB0/YtTVTJ+IAGQlOS0bFs9OP4JoKSbi1 B7yQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=QpRyhV0ioNwEBbWGEQ9+MPgX6s7F/EgYBuB/xL1fFb8=; b=etbYumV3qT1C6MD0zu5TFNMQfM9nrbdA+GSQssRfLF+dCHPoOxcreJy2n4WE+o0pHU e+tLbfu7zbCe8l6mXe3RVG4vJ4b2QOu8eXedTP+PIcEHInbdMmaMNfoOLs2HGGNwrRRs YFiwxpHQ+UeLEhx+8Ar8jJWCuR6aS54xO9KxK4Nzw57w3oKCuB8iBgPDRK362GunB4WB aao4m9t8GXDVYH0fXaiDhZqREsrriXZL+wjOwILxfJFGOQD9KfIcV4QqTMxKl6/HFpza HbjWlPHo8HSV+yeirXzCFQ37H4c0v24o93U1Ch7fXLRvoC8vhbROTpo3s5N2MvKkWYVa ai1A== X-Gm-Message-State: AA6/9RmWvxUDPuCdYp14mEr3eGyQjKqjUFG1tVMEe/vbf0vlnA3dZN9R2rvmpCKtC4ov0A== X-Received: by 10.66.230.168 with SMTP id sz8mr17177633pac.13.1475002582785; Tue, 27 Sep 2016 11:56:22 -0700 (PDT) Received: from xeon-e3 (static-50-53-69-251.bvtn.or.frontiernet.net. [50.53.69.251]) by smtp.gmail.com with ESMTPSA id y190sm6669165pfg.39.2016.09.27.11.56.22 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 27 Sep 2016 11:56:22 -0700 (PDT) Date: Tue, 27 Sep 2016 11:56:34 -0700 From: Stephen Hemminger To: "Dey, Souvik" Cc: "Kavanagh, Mark B" , Yuanhan Liu , "dev@dpdk.org" Message-ID: <20160927115634.428d1b7d@xeon-e3> In-Reply-To: References: <20160921231147.26820-1-sodey@sonusnet.com> <20160921162213.4b79d1ce@xeon-e3> <20160921184505.584367ef@xeon-e3> <20160923075320.GY23158@yliu-dev.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Sep 2016 18:56:24 -0000 On Fri, 23 Sep 2016 15:17:37 +0000 "Dey, Souvik" wrote: > Hi Liu/Mark/Stephen, > > I have tried to modify the code with all of your latest comments. Do let me know if this looks fine or you have more comments. > > > > Changes done : > > -- max frame ize is compare to VIRTIO_MAX_RX_PKTLEN instead of dev_info.max_rx_pktlen > > -- removed the CRC_LEN from the ether_len calculation and added the merge rx buf hdr len. ether_hdr_len = ETHER_HDR_LEN + VLAN_TAG_SIZE + hw->vtnet_hdr_size > > -- Still retained the VLAN Size as the worst case scenario. > > > > > > -- > > 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_SIZE 4 /* 802.3ac tag (not DMA'd) */ > > + > > +static int virtio_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) > > +{ > > + struct virtio_hw *hw = dev->data->dev_private; > > + uint32_t ether_hdr_len = ETHER_HDR_LEN + VLAN_TAG_SIZE + hw->vtnet_hdr_size; > > + uint32_t frame_size = 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); > > + 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 = { > > .allmulticast_enable = virtio_dev_allmulticast_enable, > > .allmulticast_disable = virtio_dev_allmulticast_disable, > > + .mtu_set = virtio_mtu_set, > > .dev_infos_get = virtio_dev_info_get, > > .stats_get = virtio_dev_stats_get, > > .xstats_get = virtio_dev_xstats_get, > > -- > > Please do let me know if this looks good to you all. Thanks > > > > -- > > Regards, > > Souvik > > > > -----Original Message----- > From: Kavanagh, Mark B [mailto:mark.b.kavanagh@intel.com] > Sent: Friday, September 23, 2016 5:08 AM > To: Yuanhan Liu ; Stephen Hemminger > Cc: Dey, Souvik ; dev@dpdk.org > Subject: RE: [PATCH v6] net/virtio: add set_mtu in virtio > > > > >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" > 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 > > > >> > Cc: mark.b.kavanagh@intel.com; yuanhan.liu@linux.intel.com; > > >> > dev@dpdk.org > > >> > Subject: Re: [PATCH v6] net/virtio: add set_mtu in virtio > > >> > > > >> > On Wed, 21 Sep 2016 19:11:47 -0400 > > >> > Dey > 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 = ETHER_HDR_LEN + ETHER_CRC_LEN + VLAN_TAG_SIZE; > > >> > > + uint32_t frame_size = mtu + ether_hdr_len; > > >> > > + > > >> > > + virtio_dev_info_get(dev, &dev_info); > > >> > > + > > >> > > + if (mtu < ETHER_MIN_MTU || frame_size > dev_info.max_rx_pktlen) { > > >> > > + 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 want > > >> > 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 > > >> reference 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 Ethernet 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 consider 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. > This patch is good enough for now, but not fully correct. The actual max value depends on the host negotiation protocol, ie merge rx buf is only sometimes used. There was some discussion on netdev mailing list to pass MTU from host to guest in virtio, not sure where that landed.