[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs

Wang, Liang-min liang-min.wang at intel.com
Tue Jun 2 17:47:41 CEST 2015



-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] 
Sent: Tuesday, June 02, 2015 10:33 AM
To: Wang, Liang-min
Cc: dev at 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 at 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). 



More information about the dev mailing list