From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Marchand Subject: Re: [PATCH v2 0/7] add mtu and flow control handlers Date: Tue, 17 Jun 2014 15:39:39 +0200 Message-ID: <53A0451B.6090104@6wind.com> References: <1402666663-10260-1-git-send-email-david.marchand@6wind.com> <2601191342CEEE43887BDE71AB9772580EFB73D4@IRSMSX105.ger.corp.intel.com> <539FFF5A.80307@6wind.com> <2601191342CEEE43887BDE71AB9772580EFB75F6@IRSMSX105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit To: "Ananyev, Konstantin" , "dev-VfR2kkLFssw@public.gmane.org" Return-path: In-Reply-To: <2601191342CEEE43887BDE71AB9772580EFB75F6-kPTMFJFq+rEu0RiL9chJVbfspsVTdybXVpNB7YpNyf8@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" 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