Re: [PATCH v2 0/7] add mtu and flow control handlers

[David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

On 06/17/2014 10:57 AM, Ananyev, Konstantin wrote:
> Yes, I understand that it will be initialised to 0 together with whole dev->data.
> But then, the condition:
> if (dev->data->min_rx_buf_size > mbp_buf_size)
> would never be true, and  min_rx_buf_size would always remain 0?
> I thought you need to initialise it with UINT32_MAX(or UINT16_MAX).
> BTW, not big deal, but I think uint16_t is enough for min_rx_buf_size.

- Oh, right...
We need a check on this :
if (!dev->data->min_rx_buf_size ||
     dev->data->min_rx_buf_size > mbp_buf_size)


- Yep, uint16_t should be enough for min_rx_buf_size, but then, we might 
want to update other places where bufsizes are compared to uin32_t as well.


- Actually, looking at dev->data structure, there is something 
suspicious to me.
 From what I understood, secondary processes are not supposed to touch 
dev->data, at it is shared between processes.
So I don't understand why rte_eth_dev_allocate() writes 
dev->data->port_id, without looking at process type.

Idem, later in rte_eth_dev_init(), where 
eth_dev->data->rx_mbuf_alloc_failed is set to 0 (which should already be 
set to 0 anyway).

I think a cleanup is required here but it can wait until 1.7 is out.
Plus, I am not sure we should let secondary processes use fdir calls, 
change vlan offloads etc...


>
>>>
>>> 3)  if ((mtu < 68) || (frame_size > dev_info.max_rx_pktlen))
>>> Can we add a new define for min allowable MTU (68) as it used in few places.
>
>> RTE_IPV4_MIN_MTU then ?
>
> Sounds good to me.
>
>> I am not sure where this belongs, it could go in rte_ethdev.h.
>
> Probably rte_ether.h?

Ok, I spoke to Ivan and Thomas off-list.
I propose to add the following definition in rte_ether.h :

#define ETHER_MIN_MTU 68
/**< Minimum MTU for IPv4 packets, see RFC 791. */

What do you think of this ?


-- 
David Marchand