From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2B2CAA00BE; Wed, 30 Oct 2019 05:38:20 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6FE5A1BEAF; Wed, 30 Oct 2019 05:38:19 +0100 (CET) Received: from mail-io1-f65.google.com (mail-io1-f65.google.com [209.85.166.65]) by dpdk.org (Postfix) with ESMTP id 71F821BEA8 for ; Wed, 30 Oct 2019 05:38:18 +0100 (CET) Received: by mail-io1-f65.google.com with SMTP id 1so997933iou.4 for ; Tue, 29 Oct 2019 21:38:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=dwY2Bn3LoS4EkyNNvLWpcl94xy672xZFl2wtduxruU8=; b=FrDRCnK1vK6Qg4gFWSn1B80siiKIvUi//k/zrNIdgvuQb6FfxR49eq+Td0/UaVf40H TPZrQsDYCsdkHgloL21kFBsnTmjrKXkMYqCQ1RAAELERLX4Hk1zahj+6dGe3f3YhYEDQ 78M9pf+1yvioIfm7vDPzmWN6VwqaN5kdPRLnjmzLWLNr81cUXuH6N732R2smrrHXwyZm TsH9dSJVTHuGap3lDHDeRq5NT8V6myruf8HccySv5cTxK0JRYyi/QVSjji6IX9geIiny KdlowflKDV1U0i9JLS+hk7xGqS3QkhSDYi4PxBRmr/VrxLtukHlnJG1Wv0qVQj8bfsdl EvQw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=dwY2Bn3LoS4EkyNNvLWpcl94xy672xZFl2wtduxruU8=; b=qz4oQhDRJP4SdR+oakR6uy9SBUmzsgXBL18bgZJg15UZOqIxofE9FbLwl9v69qH/9R hAM+sFC3+GNokNZfT2kemv5k6fRDkduRLM6gH+BnBna0eDsBDbnZivgZUjVnx080i0A1 7d8cXyA43x2DNQ5WRm3z+07K821wGvWd+2BxR0jT9wxM2oLLGPvrD6DcWQZv0nl3nejf 7stEepsB8QHg5fjYOIwpOruv3atBFtg22EYi95ZQrl8wltScuhh7YFgYQMXGIYD7hAfC w2Hcz83pB2lWLQ+8ZrWUsgeoiRinqQAiPnt3J2uTQWStx8HfhWaqv9OgELUrQYNYdvkp us9w== X-Gm-Message-State: APjAAAXaglr8ULL5iKwDZLvwDAGvDFrjKxMy1FnAlx73zuR8DuuaPxfV BgvrtBnHDhptX7E6J2w4GHUqTvjiaFB2OdYRrWU= X-Google-Smtp-Source: APXvYqwA3SfS8F8zORjnHsAQtK26veYU89KLElXeh52Wy0VkkRgNs/pPYQ8BH62R80d65l1zTehhB9AUnHgRvl/H3Zw= X-Received: by 2002:a6b:7609:: with SMTP id g9mr7759051iom.130.1572410297236; Tue, 29 Oct 2019 21:38:17 -0700 (PDT) MIME-Version: 1.0 References: <20191015075133.38560-1-haiyue.wang@intel.com> <1811898.7XjjD7ZjLQ@xps> <12001140.UMXFOKstgs@xps> In-Reply-To: From: Jerin Jacob Date: Wed, 30 Oct 2019 10:08:00 +0530 Message-ID: To: Ferruh Yigit Cc: Thomas Monjalon , Haiyue Wang , dpdk-dev , "Ye, Xiaolong" , "Kinsella, Ray" , Bernard Iremonger , "Sun, Chenmin" , Andrew Rybchenko , Slava Ovsiienko , Stephen Hemminger , David Marchand , Jerin Jacob Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add the API for getting burst mode information X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Oct 29, 2019 at 10:29 PM Ferruh Yigit wrote: > > On 10/26/2019 7:58 AM, Jerin Jacob wrote: > > On Sat, Oct 26, 2019 at 3:57 AM Thomas Monjalon wrote: > >> > >> 25/10/2019 18:02, Jerin Jacob: > >>> On Fri, Oct 25, 2019 at 9:15 PM Thomas Monjalon wrote: > >>>> 25/10/2019 16:08, Ferruh Yigit: > >>>>> On 10/25/2019 10:36 AM, Thomas Monjalon wrote: > >>>>>> 15/10/2019 09:51, Haiyue Wang: > >>>>>>> Some PMDs have more than one RX/TX burst paths, add the ethdev API > >>>>>>> that allows an application to retrieve the mode information about > >>>>>>> Rx/Tx packet burst such as Scalar or Vector, and Vector technology > >>>>>>> like AVX2. > >>>>>> > >>>>>> I missed this patch. I and Andrew, maintainers of ethdev, were not CC'ed. > >>>>>> Ferruh, I would expect to be Cc'ed and/or get a notification before merging. > >>>>> > >>>>> It has been discussed in the mail list and went through multiple discussions, > >>>>> patch is out since the August, +1 to cc all maintainers I missed that part, > >>>>> but when the patch is reviewed and there is no objection, why block the merge? > >>>> > >>>> I'm not saying blocking the merge. > >>>> My bad is that I missed the patch and I am asking for help with a notification > >>>> in this case. Same for Andrew I guess. > >>>> Note: it is merged in master and I am looking to improve this feature. > >>> > >>>>>>> +/** > >>>>>>> + * Ethernet device RX/TX queue packet burst mode information structure. > >>>>>>> + * Used to retrieve information about packet burst mode setting. > >>>>>>> + */ > >>>>>>> +struct rte_eth_burst_mode { > >>>>>>> + uint64_t options; > >>>>>>> +}; > >>>>>> > >>>>>> Why a struct for an integer? > >>>>> > >>>>> Again by a request from me, to not need to break the API if we need to add more > >>>>> thing in the future. > >>>> > >>>> I would replace it with a string. This is the most flexible API. > >>> > >>> IMO, Probably, best of both worlds make a good option here, > >>> as Haiyue suggested if we have an additional dev_specific[1] in structure. > >>> and when a pass to the application, let common code make final string as > >>> (options flags to string + dev_specific) > >>> > >>> options flag can be zero if PMD does not have any generic flags nor > >>> interested in such a scheme. > >>> Generic flags will help at least to have some common code. > >>> > >>> [1] > >>> struct rte_eth_burst_mode { > >>> uint64_t options; > >>> char dev_specific[128]; /* PMD has specific burst mode information */ > >>> }; > >> > >> I really don't see how we can have generic flags. > >> The flags which are proposed are just matching > >> the functions implemented in Intel PMDs. > >> And this is a complicate solution. > >> Why not just returning a name for the selected Rx/Tx mode? > > > > +1 only for the name > > > > Let me clarify my earlier proposal: > > > > 1) The public ethdev API should return only "string" i.e the flags > > SHOULD NOT be exposed as ethdev API > > i.e > > int rte_eth_tx_burst_mode_name(uint16_t port_id, uint16_t queue_id, char *name); > > > > 2) The PMD interface to the common code can be following > > > > struct eth_pmd_burst_mode { > > uint64_t options; > > char name[128]; /* PMD specific burst mode information */ > > }; > > > > typedef int (*eth_burst_mode_get_t)(struct rte_eth_dev *dev, > > uint16_t queue_id, struct eth_burst_mode *mode) > > > > 3) The implementation of rte_eth_tx_burst_mode_name() shall do optons > > flag to string converion(again internal to common code implemetation) > > and concatenate with eth_pmd_burst_mode::name > > > > This would help to reuse some of the flags to name conversion logic > > across all PMDs. > > And PMD are free to return eth_pmd_burst_mode::options as zero in > > that case final > > string only be eth_pmd_burst_mode::name. > > > > I don't see any downside with this approach and it best of both worlds. > > > > I agree it will be hard or restrictive if we want to represent the all data path > options with standardized data. > > But the free text string is good for logging, but not good if the application > will get this input and give some decision with it. I was thinking it is only for logging. But if the application needs to have some decisions with it then below scheme is the best way, Though I am not sure, allowing such decision in the application is good or not. For instance, If vectorization is AVX512, What kind of decision an application can make? If it can make some decision then, will work for the NEON case? > > To combine both two, what do you think a mixed approach, similar to what Jerin > described but both options and string is visible to application, > and make 'options' only for vectorization information which is limited and be > standardized: > > int rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id, > struct rte_eth_burst_mode *mode); > > struct rte_eth_burst_mode { > uint64_t options; // This is only for VECTORIZATION mode > char *alternate_options; > } > > since "burst_mode:options" only for vectorization, it is limited and can be easy > to consume by applications. > This means removing some data path options, like "BULK_ALLOC" from current struct. ACK > > 'rte_eth_burst_mode_option_name()' can get "struct rte_eth_burst_mode" as > parameter and convert the 'options' to string and combine into single string as > a helper function to the applications. > > And +1 to providing NULL "alternate_options" can return the size of that string. ACK > > And as we find more common/standard data path options, we can move them to the > bitfield and remove from the free text. Does it make sense? Yes, if, we need to give provide a method to take action for application else not. >