Re: [dpdk-dev] [PATCH 3/5] ethdev: add mtu accessors

[David Marchand <david.marchand@6wind.com>]

Hello Konstantin,

Mmm, good points.
I am looking into them.
I will send new patches by tomorrow (hopefully).


On 06/10/2014 07:23 PM, Ananyev, Konstantin wrote:
> Hi David,
>
>> This patch adds two new functions in ethdev api to retrieve current MTU and
>> change MTU of a port.
>> These operations have been implemented for rte_em_pmd, rte_igb_pmd and
>> rte_ixgbe_pmd.
>
>> +
>> +void ixgbe_dev_set_rx_scatter_mode(struct rte_eth_dev *dev)
>> +{
>> +	if (dev->rx_pkt_burst != ixgbe_recv_scattered_pkts) {
>> +		dev->rx_pkt_burst = ixgbe_recv_scattered_pkts;
>> +		/* we must ensure that this is done when we leave the
>> +		   function */
>> +		rte_wmb();
>> +	}
>> +}
>
> I don't think it is safe to change RX function on the fly (without calling dev_stop first).
> If before that ixgbe_recv_pkts_bulk_alloc() was used, then up to 32 mbufs could be stored internally in the stage[],
> and ixgbe_recv_scattered_pkts() doesn't have a clue about them.
> Also with ixgbe_recv_pkts_bulk_alloc(), it could be up to rx_free_thresh RX descriptors not armed yet with proper buffer addresses,
> while ixgbe_recv_scattered_pkts()  doesn't expect that - it re-arms RXD straight after it receives a packet from it.
>
> I wonder is the ability to change mtu on the fly (without dev_stop) is really needed?
> If so, then we probably can allow ixgbe_dev_set_mtu() to increase MTU only if
> new frame size is less than RX buffer size OR that device already using  ixgbe_recv_scattered_pkts().
> Something like:
> frame_size <= rx_buf_size || dev->rx_pkt_burst == ixgbe_recv_scattered_pkts
>
>
>> +static int
>> +ixgbe_dev_set_mtu(struct rte_eth_dev *dev, uint16_t *mtu)
>> +{
>> ....
>> +
>> +	/* switch to jumbo mode if needed */
>> +	if (frame_size  > ETHER_MAX_LEN) {
>
> Why ETHER_MAX_LEN?
> If we have rx_buffer big enough, we don't need to split packets even for jumbo frames.
> Shouldn't it be something like inside ixgbe_dev_rx_init():
> If (mtu + 2 * IXGBE_VLAN_TAG_SIZE) > rx_buf_size) {...
>
> Same thing for igb.
>


-- 
David Marchand