Re: [dpdk-dev] [PATCH v4]net/virtio: add mtu set in virtio

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Tue, Sep 20, 2016 at 06:42:02PM +0000, Dey, Souvik wrote:
> I have already taken care of this in v5 of the patch , If possible please review the same .

I don't think so, otherwise I would not comment here.

BTW, there is a format error: you used white space instead of TAB for
indention.

You might want to send another version, to fix above two issues. Or,
if you don't mind, I could fix them for you and apply it.

	--yliu
> 
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] 
> Sent: Tuesday, September 20, 2016 3:12 AM
> To: Kavanagh, Mark B <mark.b.kavanagh@intel.com>
> Cc: Dey, Souvik <sodey@sonusnet.com>; dev@dpdk.org; stephen@networkplumber.org
> Subject: Re: [PATCH v4]net/virtio: add mtu set in virtio
> 
> On Wed, Sep 14, 2016 at 12:15:37PM +0000, Kavanagh, Mark B wrote:
> > >
> > >>+{
> > >>+       struct rte_eth_dev_info dev_info;
> > >>+       uint32_t ether_hdr_len = ETHER_HDR_LEN + ETHER_CRC_LEN + 
> > >>+VLAN_TAG_LEN;
> > >>+       uint32_t frame_size = mtu + ether_hdr_len;
> > >>+
> > >>+       virtio_dev_info_get(dev, &dev_info);
> > >>+
> > >>+       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 ETHER_MIN_MTU, as per other DPDK drivers, or alternatively 
> > >whether '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 
> > >ETHER_MIN_MTU, i think it will be good to have the  compare statement 
> > >as If(frame_size < ETHER_MIN_MTU || frame_size > 
> > >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) length, 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?
> 
> I think you are right. At least, that's how the ixgbe PMD driver code looks like.
> 
> 	--yliu