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 C8CA9A00BE; Tue, 29 Oct 2019 13:57:00 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id BD0D41BEB9; Tue, 29 Oct 2019 13:56:59 +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 6D0F61BEB8 for ; Tue, 29 Oct 2019 13:56:58 +0100 (CET) Received: by mail-io1-f65.google.com with SMTP id w12so14598769iol.11 for ; Tue, 29 Oct 2019 05:56:58 -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=X78qdMK8xBQB8tCsKEO3EmuQSALhNS5YwOpAi6ysJV8=; b=fQb4cqPOwfHTrYbP6NNkIzN7go7pOIGA/fFoxsZ5MPAHUeTweyhU+IHW/e6/DfUMzL wJkHT5uuUA036UfDLI3AuRo1jzJgXvLOs9+o2jkWzR6CNl59fEQ9wuGK60fvJIWLjDGF StOt5pxY+TcJ1SOBU510oRAgp4k1Kvppk6yEFP9k/4yI6E7OMmBLOibs/pjQXrWRkgbf +8h+KK4axbaQ7R6C1K3iytvHbmw4g2qVzboybJg7+buneST2AiJU2O5R8kfKtWTG3RMh enYAjWKt38b0huErlsXWgo4mAOJHY45lPn2ektMw2Au7ZFwa4y248u3y/fh9xto8sK7g Cyig== 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=X78qdMK8xBQB8tCsKEO3EmuQSALhNS5YwOpAi6ysJV8=; b=ZMMJxQRSOtCOITxH0so27WpE7QyjBKzhpy8uqcz1kcXljmIzXZLpZXKbIJJVhKeIU5 0B7EVdmyYrYd3QEj5S+Hh6S2riEqpUIQSFtNWAQqre2BKN6ghKxz75owz1InJvTHDanQ o5FlCgE107eBjsRuc1ZoxLDSQ0qNsrZP7JNgdBUQaVTIUzwxlxfyQucEx70w+D5LQc+3 y3q56erLt+0v4hPUh8Nwaz+aHQmG3FEusyR1wXAt6HyzNlCPUKTC2Ua2H01pGn/T7CLW tGXLXj1mzlOFJLegUZ4sQ+13q21GIRs6iVsHSK0E7vKxDVjDtL83bDEN1WcMpAw+vcOa +SuA== X-Gm-Message-State: APjAAAVjlV03dBSfxpjET4CuyHO5Z+RA8B8H4QQeGdTsZCpjYcb7RrqQ 6ehCbWCqXa0Rga5uGsoWW50XNq+X4OFXyCNidiY= X-Google-Smtp-Source: APXvYqznjRkHKYTb0J8qJ778cxg5sX5Qzmx+22NZzBT6ctXDW43q85zkhk8cLuh/o8ijg4zrSaId+cZvevJ5RTL04Tw= X-Received: by 2002:a6b:c701:: with SMTP id x1mr3493560iof.162.1572353817419; Tue, 29 Oct 2019 05:56:57 -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: Tue, 29 Oct 2019 18:26:40 +0530 Message-ID: To: "Wang, Haiyue" Cc: Thomas Monjalon , "Yigit, Ferruh" , dpdk-dev , "Ye, Xiaolong" , "Kinsella, Ray" , "Iremonger, Bernard" , "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 4:56 PM Wang, Haiyue wrote: > > Hi Jerin, Hi Wang > > > -----Original Message----- > > From: Jerin Jacob > > Sent: Tuesday, October 29, 2019 16:35 > > To: Wang, Haiyue > > Cc: Thomas Monjalon ; Yigit, Ferruh ; dpdk-dev > > ; Ye, Xiaolong ; Kinsella, Ray ; > > Iremonger, Bernard ; Sun, Chenmin ; Andrew > > Rybchenko ; Slava Ovsiienko ; Stephen Hemminger > > ; David Marchand ; Jerin Jacob > > > > Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: add the API for getting burst mode information > > > > > > > > > > 1) We can not standardize all flags when it comes to HW specific > > > > > > > > details. We should NOT pollute public API with HW specific details. > > > > > > > > > > > > > > Currently, no detail to HW NIC specific. > > > > > > > > > > > > Yes. What if I want to add a "String" they represent a specific mode of PMD, > > > > > > so that I know what mode PMD really runs. > > > > > > It is not worth adding a flag for that in public API for HW specific notion. > > > > > > That's the problem. > > > > > > > > > > > > > > > > > > > > > 2) There is a danger if application starts taking any action based on > > > > > > > > flags. It should be only for display purpose so in that case public > > > > > > > > API should be the string to avoid misuse of the API(eventually the app > > > > > > > > will fail on some PMD > > > > > > > > if it takes any action based on the flag) > > > > > > > > > > > > > > These flags are *read only* for information. Can't image how to hack DPDK. ;-) > > > > > > > > > > > > To clarify: > > > > > > If we expose flag say RTE_ETH_BURST_SIMPLE then the application can take > > > > > > some action based on > > > > > > if (flag == RTE_ETH_BURST_SIMPLE) > > > > > > do_some_thing(); > > > > > > > > > > > > If the purpose is ONLY for "display" as info then exposing as the string will > > > > > > enable to NOT standardize i.e application can never check based on > > > > > > the string name(as it is not standardized) hence no danger. > > > > > > > > > > > > So what is the purpose of this API? Just display or are you expecting > > > > > > the application can do any action based on this? > > > > > > > > > > Oh, I see. Mainly for showing which burst rx/tx module running: > > > > > > > > If so, the public API should be as string to avoid any other interpreation of > > > > flags in application. > > > > > > > > And it makes application life easy too. > > > > > > > > > > > > > > At first, we do use string, but string contains same words. Off course, this > > > is from CPU's view. Our two PMDs string are nearly the same, so we use bit > > > instead. And people may check which CPU's vertor using. And we provide to_string > > > to help make both happy. Not sure we really make them happy. > > > > # I understand the flags can enable us to reuse some of the code and I > > agree with that. > > But it has the downside mentioned above. > > > > How about the following to have the best of both worlds. > > > > 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); > > > > How about *_str_* style ? _name kind of implies it the string. may be _mode is good as it is short. > int > rte_eth_rx_burst_mode_str_get(uint16_t port_id, uint16_t queue_id, > char *buf, int buflen) We don't need buflen as it is not known to the application. The typical pattern, we followed, in dpdk is, when function called buf as NULL then the function returns the expected size so that the application can alloc and get the buffer from ethdev layer on the next iteration. > > > > 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 */ > > }; > > > > struct eth_pmd_burst_mode { > uint64_t options; > char dev_specific[128]; /* PMD specific burst mode information */ > }; > > Still use 'dev_specific' ? Since whole structure 'struct eth_pmd_burst_mode' > will be translated into string name. Yes. options - for generic flags to reuse the code dev_specific - for PMD specific string/name > > > 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 > > > > Should be better to use the user input buffer w/ size ? Since now 'option' > is private to PMD. see above. No need for a separate argument. NULL case it can return the expected size. > int > rte_eth_rx_burst_mode_str_get(uint16_t port_id, uint16_t queue_id, > char *buf, int buflen) > > And the concatenate format : "options string, dev_specific" > ^ > If option is non-zero, and has device specific, add this ',' > as separation ? Looks good to me. > > > 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. > > > > > > > > > > > > > > > > > https://docs.fd.io/vpp/18.11/d7/d1d/plugins_2dpdk_2device_2format_8c_source.html > > > > > > > > > > s = format (s, "%Utx burst function: %s\n", > > > > > 579 format_white_space, indent + 2, > > > > > 580 ptr2sname (rte_eth_devices[xd->port_id].tx_pkt_burst)); > > > > > 581 s = format (s, "%Urx burst function: %s\n", > > > > > 582 format_white_space, indent + 2, > > > > > 583 ptr2sname (rte_eth_devices[xd->port_id].rx_pkt_burst)); > > > > > > > > > > https://docs.fd.io/vpp/18.11/d7/d1d/plugins_2dpdk_2device_2format_8c_source.html > > > > > > > > > > 488 static const char * > > > > > 489 ptr2sname (void *p) > > > > > 490 { > > > > > 491 Dl_info info = { 0 }; > > > > > 492 > > > > > 493 if (dladdr (p, &info) == 0) > > > > > 494 return 0; > > > > > 495 > > > > > 496 return info.dli_sname; > > > > > 497 } > > > > > > > > > > tx burst function: ixgbe_xmit_pkts > > > > > rx burst function: ixgbe_recv_pkts > > > > > > > > > > If the PMD's rx/tx is *static* function, 'ptr2name' returns 'nil'.