[dpdk-dev] [PATCH v6] net/virtio: add set_mtu in virtio

Dey, Souvik sodey at sonusnet.com
Thu Sep 22 04:29:18 CEST 2016


I still fail to understand how the dev_info.max_rx_pktlen is wrong here, and the hw->rx_max_pktlen is defined at all ? If you see the other driver code we have used the same way there too. Also for the vlan part isn't it better the take the worst case for the mtu lower boundary check, as that will be valid for both vlan offload on and off cases. 

-----Original Message-----
From: Stephen Hemminger [mailto:stephen at networkplumber.org] 
Sent: Wednesday, September 21, 2016 9:45 PM
To: Dey, Souvik <sodey at sonusnet.com>
Cc: mark.b.kavanagh at intel.com; yuanhan.liu at linux.intel.com; dev at dpdk.org
Subject: Re: [PATCH v6] net/virtio: add set_mtu in virtio

On Thu, 22 Sep 2016 00:08:38 +0000
"Dey, Souvik" <sodey at sonusnet.com> wrote:

> Answers inline.
> 
> --
> Regards,
> Souvik
> 
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Wednesday, September 21, 2016 7:22 PM
> To: Dey, Souvik <sodey at sonusnet.com>
> Cc: mark.b.kavanagh at intel.com; yuanhan.liu at linux.intel.com; 
> dev at dpdk.org
> Subject: Re: [PATCH v6] net/virtio: add set_mtu in virtio
> 
> On Wed, 21 Sep 2016 19:11:47 -0400
> Dey <sodey at 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 = 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.

> 
>   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.

> 
>   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.

The patch is still buggy.




More information about the dev mailing list