From: "Wang, Liang-min" <liang-min.wang@intel.com>
To: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
Date: Tue, 2 Jun 2015 15:47:41 +0000 [thread overview]
Message-ID: <B6CB929FEBC10D4FAC4BCA7EF2298E2571765AC2@FMSMSX110.amr.corp.intel.com> (raw)
In-Reply-To: <3076202.B6CvAKP4DR@xps13>
-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
Sent: Tuesday, June 02, 2015 10:33 AM
To: Wang, Liang-min
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
>Wang, hope it's clear that any new development is welcomed.
>One step before integration is to clearly explain why your code is needed. That's why a nack vote may help to discuss and decide.
>
>Comments below
>
>2015-06-02 13:15, Wang, Liang-min:
> >>2015-05-29 15:26, Liang-Min Larry Wang:
> >>> adding a new library based upon ethdev APIs to provide API's that
> >>> bear the same functionality as ethtool_ops (linux/ethtool.h) and
> >>> net_device_ops (linux/netdevice.h).
> >>>
> >>> Signed-off-by: Liang-Min Larry Wang <liang-min.wang@intel.com>
> >>> ---
> >>> MAINTAINERS | 4 +
> >>> config/common_linuxapp | 5 +
> >>> lib/Makefile | 1 +
> >>> lib/librte_ethtool/Makefile | 56 +++++++
> >>> lib/librte_ethtool/rte_ethtool.c | 155 +++++++++++++++++
> >>> lib/librte_ethtool/rte_ethtool.h | 257 +++++++++++++++++++++++++++++
> >>> lib/librte_ethtool/rte_ethtool_version.map | 18 ++
> >>> mk/rte.app.mk | 1 +
> >>
> >>NACK for several reasons:
> >>- It's unclear what benefits this ethdev wrapper is bringing
>>
>> Since ethtool is provided to assist users migrating from kernel ethtool/net_device_op based design to user-space DPDK device management. The ethtool API's are created to closely maintain its original interface, therefore this library depends on <linux/ethool.h>. To avoid pollute the existing ethdev interface, a new library is created. To minimize code replication and maintain closely 1:1 API definition with kernel space API, this interface is designed based upon available ethdev APIs and add additional dev_ops if it's necessary.
>>
> >>- There is no obvious interest (how is it supposed to be used?)
>> There are already two acknowledge on this release. Earlier comment on this patch has that " ... The API's for ethtool like things are valuable ..."
>Stephen had some doubts about the real need and 2 people from Cisco (who never contributed before) give their ack without justification.
>Saying it's "valuable" or "very useful" is not enough.
>A new library needs to demonstrate in which scenario the added-value is.
>Sorry but you have to prove that it deserves to be maintained inside the dpdk project.
Not sure if the question is the usefulness of user-space ethtool (there was request for user-space ethtool @ dpdk.org last year, right, and Steve's comment ...) or the question on putting ethtool on separate library. For the latter concern, as described, the design is to avoid polluting ethdev library by this inevitable including <linux/ethtool.h>
>> >- There is no update in the doc/ directory
>> Need more guidance on that.
>You probably have to add a new chapter in the programmer's guide.
Sure, I would help doc team adding new section into programmer's guide and other if it's necessary. The first thing is to have this API approved for release first.
> >>Other comments:
> >>- the patches are not versioned
>>
>> There is version file. Not sure what do you mean "the patches are not versioned"
>I mean there is no v2/v3 in the Subject. Please read
> http://dpdk.org/dev#send
My bad, I will address this issue on next patch
> >>- the copyright starts in 2010
>>
>> Will fix that.
>>
> >>I'm curious to understand how renaming rte_eth_dev_set_mtu() to
> >>rte_ethtool_net_change_mtu() will help anyone.
>>
>> As described, this interface is designed to provide API closely to kernel space ethtool ops and net_device_op.
>But the application still needs to adapt the code to call rte_* functions.
>So changing to rte_ethtool_net_change_mtu is equivalent to change to the existing rte_eth_dev_set_mtu. I don't see the benefit.
The benefit is single interface for users to access. Instead of looking into two different interfaces (ethtool, ether).
next prev parent reply other threads:[~2015-06-02 15:47 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-29 19:26 [dpdk-dev] [PATCH 0/2] User-space Ethtool Liang-Min Larry Wang
2015-05-29 19:26 ` [dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address Liang-Min Larry Wang
2015-06-02 10:52 ` Ananyev, Konstantin
2015-06-02 12:23 ` Thomas Monjalon
2015-06-02 14:51 ` Stephen Hemminger
2015-06-02 15:07 ` Wang, Liang-min
2015-06-02 12:23 ` Wang, Liang-min
2015-06-02 13:10 ` Ananyev, Konstantin
2015-05-29 19:26 ` [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs Liang-Min Larry Wang
2015-06-02 12:38 ` Thomas Monjalon
2015-06-02 13:15 ` Wang, Liang-min
2015-06-02 14:32 ` Thomas Monjalon
2015-06-02 15:47 ` Wang, Liang-min [this message]
2015-06-02 16:02 ` Thomas Monjalon
2015-06-02 17:06 ` Wang, Liang-min
2015-06-02 20:37 ` Thomas Monjalon
2015-06-02 20:56 ` Wang, Liang-min
2015-06-03 1:00 ` David Harton (dharton)
2015-06-03 2:09 ` Andrew Harvey (agh)
2015-06-04 14:25 ` O'Driscoll, Tim
2015-06-04 14:58 ` Stephen Hemminger
2015-06-04 22:10 ` Andrew Harvey (agh)
2015-06-05 10:46 ` Thomas Monjalon
2015-06-05 11:25 ` Wang, Liang-min
2015-06-05 12:47 ` Bruce Richardson
2015-06-05 17:24 ` Andrew Harvey (agh)
2015-06-05 21:03 ` Thomas Monjalon
2015-06-05 13:40 ` Thomas Monjalon
2015-06-05 14:20 ` Wang, Liang-min
2015-06-05 16:07 ` Andrew Harvey (agh)
2015-06-05 20:57 ` Thomas Monjalon
-- strict thread matches above, loose matches on Subject: below --
2015-05-30 0:37 [dpdk-dev] [PATCH 0/2] User-space Ethtool Liang-Min Larry Wang
2015-05-30 0:37 ` [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs Liang-Min Larry Wang
2015-05-30 15:48 ` Stephen Hemminger
2015-05-30 16:16 ` Wang, Liang-min
2015-05-30 19:26 ` Stephen Hemminger
2015-05-30 19:40 ` Wang, Liang-min
2015-05-31 16:48 ` Stephen Hemminger
2015-05-31 17:30 ` Wang, Liang-min
2015-05-31 18:31 ` Wang, Liang-min
2015-06-01 12:42 ` David Harton (dharton)
2015-05-29 13:15 [dpdk-dev] [PATCH 0/2] User-space Ethtool Liang-Min Larry Wang
2015-05-29 13:15 ` [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs Liang-Min Larry Wang
2015-05-29 15:22 ` Stephen Hemminger
2015-05-29 18:17 ` Wang, Liang-min
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B6CB929FEBC10D4FAC4BCA7EF2298E2571765AC2@FMSMSX110.amr.corp.intel.com \
--to=liang-min.wang@intel.com \
--cc=dev@dpdk.org \
--cc=thomas.monjalon@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).