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

Message ID 1432927612-12244-3-git-send-email-liang-min.wang@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Liang-Min Larry Wang May 29, 2015, 7:26 p.m. UTC
  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 +
 8 files changed, 497 insertions(+)
 create mode 100644 lib/librte_ethtool/Makefile
 create mode 100644 lib/librte_ethtool/rte_ethtool.c
 create mode 100644 lib/librte_ethtool/rte_ethtool.h
 create mode 100644 lib/librte_ethtool/rte_ethtool_version.map
  

Comments

Thomas Monjalon June 2, 2015, 12:38 p.m. UTC | #1
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
- There is no obvious interest (how is it supposed to be used?)
- There is no update in the doc/ directory

Other comments:
- the patches are not versionned
- the copyright starts in 2010

I'm curious to understand how renaming rte_eth_dev_set_mtu() to
rte_ethtool_net_change_mtu() will help anyone.
  
Liang-Min Larry Wang June 2, 2015, 1:15 p.m. UTC | #2
>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 ..."

>- There is no update in the doc/ directory
Need more guidance on that.

>Other comments:
>- the patches are not versioned

There is version file. Not sure what do you mean "the patches are not versioned"

>- 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.
  
Thomas Monjalon June 2, 2015, 2:32 p.m. UTC | #3
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.

> >- 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.

> >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

> >- 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.
  
Liang-Min Larry Wang June 2, 2015, 3:47 p.m. UTC | #4
-----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).
  
Thomas Monjalon June 2, 2015, 4:02 p.m. UTC | #5
2015-06-02 15:47, Wang, Liang-min:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
> > >>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). 

Sorry it doesn't help to understand.
Today, there is an ethdev API. Why adding an ethtool-like API would help?
  
Liang-Min Larry Wang June 2, 2015, 5:06 p.m. UTC | #6
>2015-06-02 15:47, Wang, Liang-min:
>> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
>> > >>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). 
>
>Sorry it doesn't help to understand.
>Today, there is an ethdev API. Why adding an ethtool-like API would help?

Need to understand your specific concern. Do you oppose introducing new API to support ethtool ops and net_device_ops? 
Or your concern is on the implementation. If that's latter. 
Do you oppose adding a new library to implement ethtool ops and net_device_ops?
    If so, are you satisfied with my previous answer on avoiding polluting ethdev APIs? 
        If not, do you suggest integrating ethtool APIs into ethdev API?
    If not, is your concern on the implementation of common functionality between ethtool and ethdev APIs?
        If so, as explained, it is designed to provide a unified interface to assist users to migrate from kernel ethtool/net-device-op API to DPDK
BTW, as my reply to Steve's comment, more ops are on their way. This patch is to open up the interface.
  
Thomas Monjalon June 2, 2015, 8:37 p.m. UTC | #7
I have the feeling we are not progressing in this discussion.
Please bring new explanations or I'll give up.
David Harton already acked it so maybe he could explain why it is useful.

Comments below

2015-06-02 17:06, Wang, Liang-min:
> >2015-06-02 15:47, Wang, Liang-min:
> >> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
> >> > >>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). 
> >
> >Sorry it doesn't help to understand.
> >Today, there is an ethdev API. Why adding an ethtool-like API would help?
> 
> Need to understand your specific concern. Do you oppose introducing new API to support ethtool ops and net_device_ops? 

They are not ethtool/netdev ops but fake ones.
In linux:
	int dev_set_mtu(struct net_device *dev, int new_mtu)
In dpdk:
	int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu);
So yes, I'm opposed to add an API which is neither ethdev, neither ethtool.
But I may be missing an obvious justification.

> Or your concern is on the implementation. If that's latter. 
> Do you oppose adding a new library to implement ethtool ops and net_device_ops?

Already answered above

>     If so, are you satisfied with my previous answer on avoiding polluting ethdev APIs? 
>         If not, do you suggest integrating ethtool APIs into ethdev API?

No, it's better as a separate library.

>     If not, is your concern on the implementation of common functionality between ethtool and ethdev APIs?
>         If so, as explained, it is designed to provide a unified interface to assist users to migrate from kernel ethtool/net-device-op API to DPDK

Do you mean it would help migrating some code from kernel space to dpdk?
How it would help since the functions and data are different from the kernel ones?

> BTW, as my reply to Steve's comment, more ops are on their way. This patch is to open up the interface.

Currently they are only some one-liners plus ethtool_drvinfo formatting
so there is no real benefit.
Why not wait to have more ops implemented?
  
Liang-Min Larry Wang June 2, 2015, 8:56 p.m. UTC | #8
>I have the feeling we are not progressing in this discussion.
>Please bring new explanations or I'll give up.
>David Harton already acked it so maybe he could explain why it is useful.
>
>Comments below
>
>2015-06-02 17:06, Wang, Liang-min:
>> >2015-06-02 15:47, Wang, Liang-min:
>> >> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>> >> > >>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). 
>> >
>> >Sorry it doesn't help to understand.
>> >Today, there is an ethdev API. Why adding an ethtool-like API would help?
>> 
>> Need to understand your specific concern. Do you oppose introducing new API to support ethtool ops and net_device_ops? 
>
>They are not ethtool/netdev ops but fake ones.
>In linux:
>	int dev_set_mtu(struct net_device *dev, int new_mtu) In dpdk:
>	int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu); So yes, I'm opposed to add an API which is neither ethdev, neither ethtool.
>But I may be missing an obvious justification.

So, the design purposely stays away from struct net_device to avoid carrying kernel states. We consciously choose port in place of net_device.
The kni already provides ethtool for kernel space code, this interface is designed for user-space application (fast-path comparing to kni).

>> Or your concern is on the implementation. If that's latter. 
>> Do you oppose adding a new library to implement ethtool ops and net_device_ops?
>
>Already answered above
>>
>>     If so, are you satisfied with my previous answer on avoiding polluting ethdev APIs? 
>>         If not, do you suggest integrating ethtool APIs into ethdev API?
>
>No, it's better as a separate library.
>
>>     If not, is your concern on the implementation of common functionality between ethtool and ethdev APIs?
>>         If so, as explained, it is designed to provide a unified 
>> interface to assist users to migrate from kernel ethtool/net-device-op 
>> API to DPDK
>
>Do you mean it would help migrating some code from kernel space to dpdk?
>How it would help since the functions and data are different from the kernel ones?

For application that was designed based upon kernel-space ethtool-op and net-device-op, the user-space APIs provide a path for quick integration.

>> BTW, as my reply to Steve's comment, more ops are on their way. This patch is to open up the interface.
>
>Currently they are only some one-liners plus ethtool_drvinfo formatting so there is no real benefit.
>Why not wait to have more ops implemented?
Due to amount of code change, I was advised to put into separate release and started with APIs that only requires minor changes on ethdev.
The rest of API requires changes on NIC pmd driver to support ops that are not currently supported.
  
David Harton June 3, 2015, 1 a.m. UTC | #9
Hi Thomas,

I think Larry explains things pretty clearly below.

A new application facing API is bring provided that avoids reusing kernel specific types and structs or dipping into the kernel itself.  It also clearly separates the user space API from driver related functions/types.

We do want to limit dipping into the kernel for performance reasons.

I hope this helps,
Dave

> -----Original Message-----
> From: Wang, Liang-min [mailto:liang-min.wang@intel.com]
> Sent: Tuesday, June 02, 2015 4:56 PM
> To: Thomas Monjalon
> Cc: dev@dpdk.org; David Harton (dharton)
> Subject: RE: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide
> ethtool-alike APIs
> 
> >I have the feeling we are not progressing in this discussion.
> >Please bring new explanations or I'll give up.
> >David Harton already acked it so maybe he could explain why it is useful.
> >
> >Comments below
> >
> >2015-06-02 17:06, Wang, Liang-min:
> >> >2015-06-02 15:47, Wang, Liang-min:
> >> >> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> >> >> > >>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).
> >> >
> >> >Sorry it doesn't help to understand.
> >> >Today, there is an ethdev API. Why adding an ethtool-like API would
> help?
> >>
> >> Need to understand your specific concern. Do you oppose introducing new
> API to support ethtool ops and net_device_ops?
> >
> >They are not ethtool/netdev ops but fake ones.
> >In linux:
> >	int dev_set_mtu(struct net_device *dev, int new_mtu) In dpdk:
> >	int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu); So yes,
> I'm opposed to add an API which is neither ethdev, neither ethtool.
> >But I may be missing an obvious justification.
> 
> So, the design purposely stays away from struct net_device to avoid
> carrying kernel states. We consciously choose port in place of net_device.
> The kni already provides ethtool for kernel space code, this interface is
> designed for user-space application (fast-path comparing to kni).
> 
> >> Or your concern is on the implementation. If that's latter.
> >> Do you oppose adding a new library to implement ethtool ops and
> net_device_ops?
> >
> >Already answered above
> >>
> >>     If so, are you satisfied with my previous answer on avoiding
> polluting ethdev APIs?
> >>         If not, do you suggest integrating ethtool APIs into ethdev
> API?
> >
> >No, it's better as a separate library.
> >
> >>     If not, is your concern on the implementation of common
> functionality between ethtool and ethdev APIs?
> >>         If so, as explained, it is designed to provide a unified
> >> interface to assist users to migrate from kernel
> >> ethtool/net-device-op API to DPDK
> >
> >Do you mean it would help migrating some code from kernel space to dpdk?
> >How it would help since the functions and data are different from the
> kernel ones?
> 
> For application that was designed based upon kernel-space ethtool-op and
> net-device-op, the user-space APIs provide a path for quick integration.
> 
> >> BTW, as my reply to Steve's comment, more ops are on their way. This
> patch is to open up the interface.
> >
> >Currently they are only some one-liners plus ethtool_drvinfo formatting
> so there is no real benefit.
> >Why not wait to have more ops implemented?
> Due to amount of code change, I was advised to put into separate release
> and started with APIs that only requires minor changes on ethdev.
> The rest of API requires changes on NIC pmd driver to support ops that are
> not currently supported.
  
Andrew Harvey (agh) June 3, 2015, 2:09 a.m. UTC | #10
I believe that their is value in this interface for software stacks not
based on Linux being moved toward DPDK that need simple operations like
getting the mac address.  Some of these stacks have a dearth of resources
available and dedicating a core/thread to KNI to get/set a mac address
is considered excessive. There are also issues with 32/64 bit kernel
integration
using KNI.  If the ethtool interface is not the correct interface then
please help me
understand what should/could have been used. If ethtool is considered 'old
and clunky¹
Stephen's and your input would be valuable in designing another interface
with
similar properties.  The use-case is pretty simple and there is no plans
for moving
anything back into the kernel on the contrary its the complete opposite.

‹ Andy


On 6/2/15, 1:37 PM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:

>I have the feeling we are not progressing in this discussion.
>Please bring new explanations or I'll give up.
>David Harton already acked it so maybe he could explain why it is useful.
>
>Comments below
>
>2015-06-02 17:06, Wang, Liang-min:
>> >2015-06-02 15:47, Wang, Liang-min:
>> >> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>> >> > >>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).
>> >
>> >Sorry it doesn't help to understand.
>> >Today, there is an ethdev API. Why adding an ethtool-like API would
>>help?
>> 
>> Need to understand your specific concern. Do you oppose introducing new
>>API to support ethtool ops and net_device_ops?
>
>They are not ethtool/netdev ops but fake ones.
>In linux:
>	int dev_set_mtu(struct net_device *dev, int new_mtu)
>In dpdk:
>	int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu);
>So yes, I'm opposed to add an API which is neither ethdev, neither
>ethtool.
>But I may be missing an obvious justification.
>
>> Or your concern is on the implementation. If that's latter.
>> Do you oppose adding a new library to implement ethtool ops and
>>net_device_ops?
>
>Already answered above
>
>>     If so, are you satisfied with my previous answer on avoiding
>>polluting ethdev APIs?
>>         If not, do you suggest integrating ethtool APIs into ethdev API?
>
>No, it's better as a separate library.
>
>>     If not, is your concern on the implementation of common
>>functionality between ethtool and ethdev APIs?
>>         If so, as explained, it is designed to provide a unified
>>interface to assist users to migrate from kernel ethtool/net-device-op
>>API to DPDK
>
>Do you mean it would help migrating some code from kernel space to dpdk?
>How it would help since the functions and data are different from the
>kernel ones?
>
>> BTW, as my reply to Steve's comment, more ops are on their way. This
>>patch is to open up the interface.
>
>Currently they are only some one-liners plus ethtool_drvinfo formatting
>so there is no real benefit.
>Why not wait to have more ops implemented?
>
  
O'driscoll, Tim June 4, 2015, 2:25 p.m. UTC | #11
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andrew Harvey (agh)
> Sent: Wednesday, June 3, 2015 3:10 AM
> To: Thomas Monjalon; Wang, Liang-min
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide
> ethtool-alike APIs
> 
> I believe that their is value in this interface for software stacks not
> based on Linux being moved toward DPDK that need simple operations like
> getting the mac address.  Some of these stacks have a dearth of
> resources
> available and dedicating a core/thread to KNI to get/set a mac address
> is considered excessive. There are also issues with 32/64 bit kernel
> integration
> using KNI.  If the ethtool interface is not the correct interface then
> please help me
> understand what should/could have been used. If ethtool is considered
> 'old
> and clunky¹
> Stephen's and your input would be valuable in designing another
> interface
> with
> similar properties.  The use-case is pretty simple and there is no plans
> for moving
> anything back into the kernel on the contrary its the complete opposite.
> 
> < Andy

Stephen, Thomas: I think it was the two of you that originally questioned the justification for including this change. Now that Andy and Dave Harton have clarified, does this resolve your concerns or do you still have questions? I just want to make sure that we reach a timely conclusion on this.


Tim

> 
> 
> On 6/2/15, 1:37 PM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:
> 
> >I have the feeling we are not progressing in this discussion.
> >Please bring new explanations or I'll give up.
> >David Harton already acked it so maybe he could explain why it is
> useful.
> >
> >Comments below
> >
> >2015-06-02 17:06, Wang, Liang-min:
> >> >2015-06-02 15:47, Wang, Liang-min:
> >> >> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> >> >> > >>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).
> >> >
> >> >Sorry it doesn't help to understand.
> >> >Today, there is an ethdev API. Why adding an ethtool-like API would
> >>help?
> >>
> >> Need to understand your specific concern. Do you oppose introducing
> new
> >>API to support ethtool ops and net_device_ops?
> >
> >They are not ethtool/netdev ops but fake ones.
> >In linux:
> >	int dev_set_mtu(struct net_device *dev, int new_mtu)
> >In dpdk:
> >	int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu);
> >So yes, I'm opposed to add an API which is neither ethdev, neither
> >ethtool.
> >But I may be missing an obvious justification.
> >
> >> Or your concern is on the implementation. If that's latter.
> >> Do you oppose adding a new library to implement ethtool ops and
> >>net_device_ops?
> >
> >Already answered above
> >
> >>     If so, are you satisfied with my previous answer on avoiding
> >>polluting ethdev APIs?
> >>         If not, do you suggest integrating ethtool APIs into ethdev
> API?
> >
> >No, it's better as a separate library.
> >
> >>     If not, is your concern on the implementation of common
> >>functionality between ethtool and ethdev APIs?
> >>         If so, as explained, it is designed to provide a unified
> >>interface to assist users to migrate from kernel ethtool/net-device-op
> >>API to DPDK
> >
> >Do you mean it would help migrating some code from kernel space to
> dpdk?
> >How it would help since the functions and data are different from the
> >kernel ones?
> >
> >> BTW, as my reply to Steve's comment, more ops are on their way. This
> >>patch is to open up the interface.
> >
> >Currently they are only some one-liners plus ethtool_drvinfo formatting
> >so there is no real benefit.
> >Why not wait to have more ops implemented?
> >
  
Stephen Hemminger June 4, 2015, 2:58 p.m. UTC | #12
On Wed, 3 Jun 2015 02:09:39 +0000
"Andrew Harvey (agh)" <agh@cisco.com> wrote:

> I believe that their is value in this interface for software stacks not
> based on Linux being moved toward DPDK that need simple operations like
> getting the mac address.  Some of these stacks have a dearth of resources
> available and dedicating a core/thread to KNI to get/set a mac address
> is considered excessive. There are also issues with 32/64 bit kernel
> integration
> using KNI.  If the ethtool interface is not the correct interface then
> please help me
> understand what should/could have been used. If ethtool is considered 'old
> and clunky¹
> Stephen's and your input would be valuable in designing another interface
> with
> similar properties.  The use-case is pretty simple and there is no plans
> for moving
> anything back into the kernel on the contrary its the complete opposite.
> 
> ‹ Andy

We have DPDK API's to do this, and any added wrappers make it bigger.
I don't see why calling your ethtool API is better than calling
rte_eth* API.

If there is a missing functionality in the rte_ethXXX api's for an
application then add that. For example: rte_eth_mac_addr_get()
  
Andrew Harvey (agh) June 4, 2015, 10:10 p.m. UTC | #13
On 6/4/15, 7:58 AM, "Stephen Hemminger" <stephen@networkplumber.org> wrote:

>On Wed, 3 Jun 2015 02:09:39 +0000

>"Andrew Harvey (agh)" <agh@cisco.com> wrote:

>

>> I believe that their is value in this interface for software stacks not

>> based on Linux being moved toward DPDK that need simple operations like

>> getting the mac address.  Some of these stacks have a dearth of

>>resources

>> available and dedicating a core/thread to KNI to get/set a mac address

>> is considered excessive. There are also issues with 32/64 bit kernel

>> integration

>> using KNI.  If the ethtool interface is not the correct interface then

>> please help me

>> understand what should/could have been used. If ethtool is considered

>>'old

>> and clunky¹

>> Stephen's and your input would be valuable in designing another

>>interface

>> with

>> similar properties.  The use-case is pretty simple and there is no plans

>> for moving

>> anything back into the kernel on the contrary its the complete opposite.

>> 

>> ‹ Andy

>

>We have DPDK API's to do this, and any added wrappers make it bigger.

>I don't see why calling your ethtool API is better than calling

>rte_eth* API.

>

>If there is a missing functionality in the rte_ethXXX api's for an

>application then add that. For example: rte_eth_mac_addr_get()


I am getting somewhat confused by your latest comments.  Your first email
(referenced below) looked really positive and I found your suggestions
useful. Your latest post appears to contradict this and now the interface
was there all the time.  The wrapper façade provided by the ethtool
library provide a clean separation of concerns and will allow people to
migrate from not only KNI but in our case from a legacy system.  If a
software stack has requirements to work with multiple IO abstractions
then the ethtool approach is attractive. I would speculate that many
other stacks moving towards dpdk will have similar issues.

Summarizing, for our use-cases the ethtool interface facilitated our
adoption to dpdk while allowing us to support our legacy IO abstractions.

— Andy

http://dpdk.org/ml/archives/dev/2015-May/018408.html (original email)

http://dpdk.org/ml/archives/dev/2014-June/003005.html (ethtool request)
  
Thomas Monjalon June 5, 2015, 10:46 a.m. UTC | #14
2015-06-04 22:10, Andrew Harvey:
> On 6/4/15, 7:58 AM, "Stephen Hemminger" <stephen@networkplumber.org> wrote:
> >"Andrew Harvey (agh)" <agh@cisco.com> wrote:
> >> I believe that their is value in this interface for software stacks not
> >> based on Linux being moved toward DPDK that need simple operations like
> >> getting the mac address.  Some of these stacks have a dearth of
> >>resources
> >> available and dedicating a core/thread to KNI to get/set a mac address
> >> is considered excessive. There are also issues with 32/64 bit kernel
> >> integration
> >> using KNI.  If the ethtool interface is not the correct interface then
> >> please help me
> >> understand what should/could have been used. If ethtool is considered
> >>'old
> >> and clunky¹
> >> Stephen's and your input would be valuable in designing another
> >>interface
> >> with
> >> similar properties.  The use-case is pretty simple and there is no plans
> >> for moving
> >> anything back into the kernel on the contrary its the complete opposite.
> >> 
> >> ‹ Andy
> >
> >We have DPDK API's to do this, and any added wrappers make it bigger.
> >I don't see why calling your ethtool API is better than calling
> >rte_eth* API.
> >
> >If there is a missing functionality in the rte_ethXXX api's for an
> >application then add that. For example: rte_eth_mac_addr_get()
> 
> I am getting somewhat confused by your latest comments.  Your first email
> (referenced below) looked really positive and I found your suggestions
> useful. Your latest post appears to contradict this and now the interface
> was there all the time.  The wrapper façade provided by the ethtool
> library provide a clean separation of concerns and will allow people to
> migrate from not only KNI but in our case from a legacy system.  If a
> software stack has requirements to work with multiple IO abstractions
> then the ethtool approach is attractive. I would speculate that many
> other stacks moving towards dpdk will have similar issues.
> 
> Summarizing, for our use-cases the ethtool interface facilitated our
> adoption to dpdk while allowing us to support our legacy IO abstractions.

Stephen and me say the same thing about using the ethdev API.
We don't understand why using a fake ethtool lib would be easier.
Though you are saying it "facilitated [your] adoption to dpdk".
Please could you explain why using an ethtool-like API is easier than
using the existing ethdev API?
In any case, you have to develop a specific backend for DPDK
(rte_ethtool would be also DPDK-specific).

It seems you already started to use such an ethtool implementation.
Please note that our goal is not to prevent Cisco from upstreaming
(evidence with enic driver integration) but we want to guide you, and
others having the same needs, to the best solution for everybody.
That's why we need to understand what we (or you) are missing.
Maybe that it would be clearer with some code examples (which would
go in the lib documentation if any).

Thanks
  
Liang-Min Larry Wang June 5, 2015, 11:25 a.m. UTC | #15
> -----Original Message-----

> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]

> Sent: Friday, June 05, 2015 6:47 AM

> To: Andrew Harvey (agh)

> Cc: Stephen Hemminger; Wang, Liang-min; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide

> ethtool-alike APIs

> 

> 2015-06-04 22:10, Andrew Harvey:

> > On 6/4/15, 7:58 AM, "Stephen Hemminger"

> <stephen@networkplumber.org> wrote:

> > >"Andrew Harvey (agh)" <agh@cisco.com> wrote:

> > >> I believe that their is value in this interface for software stacks

> > >>not  based on Linux being moved toward DPDK that need simple

> > >>operations like  getting the mac address.  Some of these stacks have

> > >>a dearth of resources  available and dedicating a core/thread to KNI

> > >>to get/set a mac address  is considered excessive. There are also

> > >>issues with 32/64 bit kernel  integration  using KNI.  If the

> > >>ethtool interface is not the correct interface then  please help me

> > >>understand what should/could have been used. If ethtool is

> > >>considered 'old  and clunky¹  Stephen's and your input would be

> > >>valuable in designing another interface  with  similar properties.

> > >>The use-case is pretty simple and there is no plans  for moving

> > >>anything back into the kernel on the contrary its the complete opposite.

> > >>

> > >> ‹ Andy

> > >

> > >We have DPDK API's to do this, and any added wrappers make it bigger.

> > >I don't see why calling your ethtool API is better than calling

> > >rte_eth* API.

> > >

> > >If there is a missing functionality in the rte_ethXXX api's for an

> > >application then add that. For example: rte_eth_mac_addr_get()

> >

> > I am getting somewhat confused by your latest comments.  Your first

> > email (referenced below) looked really positive and I found your

> > suggestions useful. Your latest post appears to contradict this and

> > now the interface was there all the time.  The wrapper façade provided

> > by the ethtool library provide a clean separation of concerns and will

> > allow people to migrate from not only KNI but in our case from a

> > legacy system.  If a software stack has requirements to work with

> > multiple IO abstractions then the ethtool approach is attractive. I

> > would speculate that many other stacks moving towards dpdk will have

> similar issues.

> >

> > Summarizing, for our use-cases the ethtool interface facilitated our

> > adoption to dpdk while allowing us to support our legacy IO abstractions.

> 

> Stephen and me say the same thing about using the ethdev API.

> We don't understand why using a fake ethtool lib would be easier.

> Though you are saying it "facilitated [your] adoption to dpdk".

> Please could you explain why using an ethtool-like API is easier than using

> the existing ethdev API?

> In any case, you have to develop a specific backend for DPDK (rte_ethtool

> would be also DPDK-specific).


As described earlier in this patch comment reply, there are other ethtool ops that have been implemented.
Those ops includes set/get eeprom, set/get pauseparam, set/get ringparam which are not available in the exiting ethdev library.
For this release, we focus on releasing some basic functions (btw, mac_addr_set is not available but is covered by this patch).
The key reason that this set of library is not released as part of ethdev is the ethtool API dependency on kernel include file.
To faithfully carry the ethtool ops and net dev ops API parameters, the ethtool APIs are designed to follow the original definition except avoiding carry kernel states.
With that, to support ethtool APIs faithfully, we need to include <linux/ethtool.h>. 
As suggested by many DPDK veterans including Thomas (indicated over your reply), you would prefer these APIs in a separate library.

> 

> It seems you already started to use such an ethtool implementation.

> Please note that our goal is not to prevent Cisco from upstreaming (evidence

> with enic driver integration) but we want to guide you, and others having the

> same needs, to the best solution for everybody.

> That's why we need to understand what we (or you) are missing.

> Maybe that it would be clearer with some code examples (which would go in

> the lib documentation if any).

> 

> Thanks
  
Bruce Richardson June 5, 2015, 12:47 p.m. UTC | #16
On Fri, Jun 05, 2015 at 11:25:09AM +0000, Wang, Liang-min wrote:
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Friday, June 05, 2015 6:47 AM
> > To: Andrew Harvey (agh)
> > Cc: Stephen Hemminger; Wang, Liang-min; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide
> > ethtool-alike APIs
> > 
> > 2015-06-04 22:10, Andrew Harvey:
> > > On 6/4/15, 7:58 AM, "Stephen Hemminger"
> > <stephen@networkplumber.org> wrote:
> > > >"Andrew Harvey (agh)" <agh@cisco.com> wrote:
> > > >> I believe that their is value in this interface for software stacks
> > > >>not  based on Linux being moved toward DPDK that need simple
> > > >>operations like  getting the mac address.  Some of these stacks have
> > > >>a dearth of resources  available and dedicating a core/thread to KNI
> > > >>to get/set a mac address  is considered excessive. There are also
> > > >>issues with 32/64 bit kernel  integration  using KNI.  If the
> > > >>ethtool interface is not the correct interface then  please help me
> > > >>understand what should/could have been used. If ethtool is
> > > >>considered 'old  and clunky¹  Stephen's and your input would be
> > > >>valuable in designing another interface  with  similar properties.
> > > >>The use-case is pretty simple and there is no plans  for moving
> > > >>anything back into the kernel on the contrary its the complete opposite.
> > > >>
> > > >> ‹ Andy
> > > >
> > > >We have DPDK API's to do this, and any added wrappers make it bigger.
> > > >I don't see why calling your ethtool API is better than calling
> > > >rte_eth* API.
> > > >
> > > >If there is a missing functionality in the rte_ethXXX api's for an
> > > >application then add that. For example: rte_eth_mac_addr_get()
> > >
> > > I am getting somewhat confused by your latest comments.  Your first
> > > email (referenced below) looked really positive and I found your
> > > suggestions useful. Your latest post appears to contradict this and
> > > now the interface was there all the time.  The wrapper façade provided
> > > by the ethtool library provide a clean separation of concerns and will
> > > allow people to migrate from not only KNI but in our case from a
> > > legacy system.  If a software stack has requirements to work with
> > > multiple IO abstractions then the ethtool approach is attractive. I
> > > would speculate that many other stacks moving towards dpdk will have
> > similar issues.
> > >
> > > Summarizing, for our use-cases the ethtool interface facilitated our
> > > adoption to dpdk while allowing us to support our legacy IO abstractions.
> > 
> > Stephen and me say the same thing about using the ethdev API.
> > We don't understand why using a fake ethtool lib would be easier.
> > Though you are saying it "facilitated [your] adoption to dpdk".
> > Please could you explain why using an ethtool-like API is easier than using
> > the existing ethdev API?
> > In any case, you have to develop a specific backend for DPDK (rte_ethtool
> > would be also DPDK-specific).
> 
> As described earlier in this patch comment reply, there are other ethtool ops that have been implemented.
> Those ops includes set/get eeprom, set/get pauseparam, set/get ringparam which are not available in the exiting ethdev library.
> For this release, we focus on releasing some basic functions (btw, mac_addr_set is not available but is covered by this patch).
> The key reason that this set of library is not released as part of ethdev is the ethtool API dependency on kernel include file.
> To faithfully carry the ethtool ops and net dev ops API parameters, the ethtool APIs are designed to follow the original definition except avoiding carry kernel states.
> With that, to support ethtool APIs faithfully, we need to include <linux/ethtool.h>. 
> As suggested by many DPDK veterans including Thomas (indicated over your reply), you would prefer these APIs in a separate library.
> 
> > 
> > It seems you already started to use such an ethtool implementation.
> > Please note that our goal is not to prevent Cisco from upstreaming (evidence
> > with enic driver integration) but we want to guide you, and others having the
> > same needs, to the best solution for everybody.
> > That's why we need to understand what we (or you) are missing.
> > Maybe that it would be clearer with some code examples (which would go in
> > the lib documentation if any).
> > 
> > Thanks

How about doing this work as a sample application initially, to demonstrate how
an application written using ethtool APIs could be shimmed to use DPDK underneath.
The ethtool to dpdk mapping could be contained in a single header file (or header
and c file) inside the sample app. This would allow easy re-use of the shim
layer, while at the same time not making it part of the core DPDK libraries.

Regards,
/Bruce
  
Thomas Monjalon June 5, 2015, 1:40 p.m. UTC | #17
2015-06-05 11:25, Wang, Liang-min:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Stephen and me say the same thing about using the ethdev API.
> > We don't understand why using a fake ethtool lib would be easier.
> > Though you are saying it "facilitated [your] adoption to dpdk".
> > Please could you explain why using an ethtool-like API is easier than using
> > the existing ethdev API?
> > In any case, you have to develop a specific backend for DPDK (rte_ethtool
> > would be also DPDK-specific).
> 
> As described earlier in this patch comment reply, there are other ethtool ops that have been implemented.
> Those ops includes set/get eeprom, set/get pauseparam, set/get ringparam which are not available in the exiting ethdev library.

1/ We cannot really consider code which is not public
2/ You may extend ethdev if some functions are missing

> For this release, we focus on releasing some basic functions (btw, mac_addr_set is not available but is covered by this patch).

Yes, you are extending ethdev by adding rte_eth_dev_default_mac_addr_set.

> The key reason that this set of library is not released as part of ethdev is the ethtool API dependency on kernel include file.

It is a good reason to separate the library.
But it doesn't justify its need.

> To faithfully carry the ethtool ops and net dev ops API parameters, the ethtool APIs are designed to follow the original definition except avoiding carry kernel states.
> With that, to support ethtool APIs faithfully, we need to include <linux/ethtool.h>. 
> As suggested by many DPDK veterans including Thomas (indicated over your reply), you would prefer these APIs in a separate library.

I think I'm starting to understand that you really need ethtool conversion
(implemented in rte_ethtool_get_drvinfo) but not the other functions which
are simple wrappers. Right?
  
Liang-Min Larry Wang June 5, 2015, 2:20 p.m. UTC | #18
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, June 05, 2015 9:41 AM
> To: Wang, Liang-min
> Cc: Andrew Harvey (agh); Stephen Hemminger; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide
> ethtool-alike APIs
> 
> 2015-06-05 11:25, Wang, Liang-min:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > Stephen and me say the same thing about using the ethdev API.
> > > We don't understand why using a fake ethtool lib would be easier.
> > > Though you are saying it "facilitated [your] adoption to dpdk".
> > > Please could you explain why using an ethtool-like API is easier
> > > than using the existing ethdev API?
> > > In any case, you have to develop a specific backend for DPDK
> > > (rte_ethtool would be also DPDK-specific).
> >
> > As described earlier in this patch comment reply, there are other ethtool
> ops that have been implemented.
> > Those ops includes set/get eeprom, set/get pauseparam, set/get
> ringparam which are not available in the exiting ethdev library.
> 
> 1/ We cannot really consider code which is not public 2/ You may extend
> ethdev if some functions are missing
> 
> > For this release, we focus on releasing some basic functions (btw,
> mac_addr_set is not available but is covered by this patch).
> 
> Yes, you are extending ethdev by adding
> rte_eth_dev_default_mac_addr_set.
> 
> > The key reason that this set of library is not released as part of ethdev is
> the ethtool API dependency on kernel include file.
> 
> It is a good reason to separate the library.
> But it doesn't justify its need.
> 
> > To faithfully carry the ethtool ops and net dev ops API parameters, the
> ethtool APIs are designed to follow the original definition except avoiding
> carry kernel states.
> > With that, to support ethtool APIs faithfully, we need to include
> <linux/ethtool.h>.
> > As suggested by many DPDK veterans including Thomas (indicated over
> your reply), you would prefer these APIs in a separate library.
> 
> I think I'm starting to understand that you really need ethtool conversion
> (implemented in rte_ethtool_get_drvinfo) but not the other functions which
> are simple wrappers. Right?

The rte_ethtool_get_drvinfo and many others ethtool ops have the same conversion requirement.
As for ethtool and net dev ops that don't require conversion. For the sake of clean API interface, they are implemented in the same ethtool library.
  
Andrew Harvey (agh) June 5, 2015, 4:07 p.m. UTC | #19
On 6/5/15, 3:46 AM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:

>2015-06-04 22:10, Andrew Harvey:

>> On 6/4/15, 7:58 AM, "Stephen Hemminger" <stephen@networkplumber.org>

>>wrote:

>> >"Andrew Harvey (agh)" <agh@cisco.com> wrote:

>> >> I believe that their is value in this interface for software stacks

>>not

>> >> based on Linux being moved toward DPDK that need simple operations

>>like

>> >> getting the mac address.  Some of these stacks have a dearth of

>> >>resources

>> >> available and dedicating a core/thread to KNI to get/set a mac

>>address

>> >> is considered excessive. There are also issues with 32/64 bit kernel

>> >> integration

>> >> using KNI.  If the ethtool interface is not the correct interface

>>then

>> >> please help me

>> >> understand what should/could have been used. If ethtool is considered

>> >>'old

>> >> and clunky¹

>> >> Stephen's and your input would be valuable in designing another

>> >>interface

>> >> with

>> >> similar properties.  The use-case is pretty simple and there is no

>>plans

>> >> for moving

>> >> anything back into the kernel on the contrary its the complete

>>opposite.

>> >> 

>> >> ‹ Andy

>> >

>> >We have DPDK API's to do this, and any added wrappers make it bigger.

>> >I don't see why calling your ethtool API is better than calling

>> >rte_eth* API.

>> >

>> >If there is a missing functionality in the rte_ethXXX api's for an

>> >application then add that. For example: rte_eth_mac_addr_get()

>> 

>> I am getting somewhat confused by your latest comments.  Your first

>>email

>> (referenced below) looked really positive and I found your suggestions

>> useful. Your latest post appears to contradict this and now the

>>interface

>> was there all the time.  The wrapper façade provided by the ethtool

>> library provide a clean separation of concerns and will allow people to

>> migrate from not only KNI but in our case from a legacy system.  If a

>> software stack has requirements to work with multiple IO abstractions

>> then the ethtool approach is attractive. I would speculate that many

>> other stacks moving towards dpdk will have similar issues.

>> 

>> Summarizing, for our use-cases the ethtool interface facilitated our

>> adoption to dpdk while allowing us to support our legacy IO

>>abstractions.

>

>Stephen and me say the same thing about using the ethdev API.


And your would have a point would be valid if dpdk were available to every
interface we support (it is not) and on every processor architecture that
we support (it is not) and every OS we support (it is not).  So to
minimize entropy in the code why not leave the client code the same
ioctl(fd, …) and hide the implementation
detail in a wrapper library.  We have a large legacy code base to move
forward and sprinkling special interest code like rte_xxx throughout every
client we have is not appropriate at this time.

 
>We don't understand why using a fake ethtool lib would be easier.

>Though you are saying it "facilitated [your] adoption to dpdk".

>Please could you explain why using an ethtool-like API is easier than

>using the existing ethdev API?

>In any case, you have to develop a specific backend for DPDK

>(rte_ethtool would be also DPDK-specific).

>

>It seems you already started to use such an ethtool implementation.

>Please note that our goal is not to prevent Cisco from upstreaming

>(evidence with enic driver integration) but we want to guide you, and

>others having the same needs, to the best solution for everybody.

>That's why we need to understand what we (or you) are missing.

>Maybe that it would be clearer with some code examples (which would

>go in the lib documentation if any).

>

>Thanks
  
Andrew Harvey (agh) June 5, 2015, 5:24 p.m. UTC | #20
On 6/5/15, 5:47 AM, "Bruce Richardson" <bruce.richardson@intel.com> wrote:

>On Fri, Jun 05, 2015 at 11:25:09AM +0000, Wang, Liang-min wrote:

>> 

>> 

>> > -----Original Message-----

>> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]

>> > Sent: Friday, June 05, 2015 6:47 AM

>> > To: Andrew Harvey (agh)

>> > Cc: Stephen Hemminger; Wang, Liang-min; dev@dpdk.org

>> > Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to

>>provide

>> > ethtool-alike APIs

>> > 

>> > 2015-06-04 22:10, Andrew Harvey:

>> > > On 6/4/15, 7:58 AM, "Stephen Hemminger"

>> > <stephen@networkplumber.org> wrote:

>> > > >"Andrew Harvey (agh)" <agh@cisco.com> wrote:

>> > > >> I believe that their is value in this interface for software

>>stacks

>> > > >>not  based on Linux being moved toward DPDK that need simple

>> > > >>operations like  getting the mac address.  Some of these stacks

>>have

>> > > >>a dearth of resources  available and dedicating a core/thread to

>>KNI

>> > > >>to get/set a mac address  is considered excessive. There are also

>> > > >>issues with 32/64 bit kernel  integration  using KNI.  If the

>> > > >>ethtool interface is not the correct interface then  please help

>>me

>> > > >>understand what should/could have been used. If ethtool is

>> > > >>considered 'old  and clunky¹  Stephen's and your input would be

>> > > >>valuable in designing another interface  with  similar properties.

>> > > >>The use-case is pretty simple and there is no plans  for moving

>> > > >>anything back into the kernel on the contrary its the complete

>>opposite.

>> > > >>

>> > > >> ‹ Andy

>> > > >

>> > > >We have DPDK API's to do this, and any added wrappers make it

>>bigger.

>> > > >I don't see why calling your ethtool API is better than calling

>> > > >rte_eth* API.

>> > > >

>> > > >If there is a missing functionality in the rte_ethXXX api's for an

>> > > >application then add that. For example: rte_eth_mac_addr_get()

>> > >

>> > > I am getting somewhat confused by your latest comments.  Your first

>> > > email (referenced below) looked really positive and I found your

>> > > suggestions useful. Your latest post appears to contradict this and

>> > > now the interface was there all the time.  The wrapper façade

>>provided

>> > > by the ethtool library provide a clean separation of concerns and

>>will

>> > > allow people to migrate from not only KNI but in our case from a

>> > > legacy system.  If a software stack has requirements to work with

>> > > multiple IO abstractions then the ethtool approach is attractive. I

>> > > would speculate that many other stacks moving towards dpdk will have

>> > similar issues.

>> > >

>> > > Summarizing, for our use-cases the ethtool interface facilitated our

>> > > adoption to dpdk while allowing us to support our legacy IO

>>abstractions.

>> > 

>> > Stephen and me say the same thing about using the ethdev API.

>> > We don't understand why using a fake ethtool lib would be easier.

>> > Though you are saying it "facilitated [your] adoption to dpdk".

>> > Please could you explain why using an ethtool-like API is easier than

>>using

>> > the existing ethdev API?

>> > In any case, you have to develop a specific backend for DPDK

>>(rte_ethtool

>> > would be also DPDK-specific).

>> 

>> As described earlier in this patch comment reply, there are other

>>ethtool ops that have been implemented.

>> Those ops includes set/get eeprom, set/get pauseparam, set/get

>>ringparam which are not available in the exiting ethdev library.

>> For this release, we focus on releasing some basic functions (btw,

>>mac_addr_set is not available but is covered by this patch).

>> The key reason that this set of library is not released as part of

>>ethdev is the ethtool API dependency on kernel include file.

>> To faithfully carry the ethtool ops and net dev ops API parameters, the

>>ethtool APIs are designed to follow the original definition except

>>avoiding carry kernel states.

>> With that, to support ethtool APIs faithfully, we need to include

>><linux/ethtool.h>.

>> As suggested by many DPDK veterans including Thomas (indicated over

>>your reply), you would prefer these APIs in a separate library.

>> 

>> > 

>> > It seems you already started to use such an ethtool implementation.

>> > Please note that our goal is not to prevent Cisco from upstreaming

>>(evidence

>> > with enic driver integration) but we want to guide you, and others

>>having the

>> > same needs, to the best solution for everybody.

>> > That's why we need to understand what we (or you) are missing.

>> > Maybe that it would be clearer with some code examples (which would

>>go in

>> > the lib documentation if any).

>> > 

>> > Thanks

>

>How about doing this work as a sample application initially, to

>demonstrate how

>an application written using ethtool APIs could be shimmed to use DPDK

>underneath.

>The ethtool to dpdk mapping could be contained in a single header file

>(or header

>and c file) inside the sample app. This would allow easy re-use of the

>shim

>layer, while at the same time not making it part of the core DPDK

>libraries.

>

>Regards,

>/Bruce


This would appear to be the most pragmatic way forward.  It would allow
others to see more of the code and judge its value for themselves. I have
no issues with this approach if others agree.

— Andy
  
Thomas Monjalon June 5, 2015, 8:57 p.m. UTC | #21
2015-06-05 16:07, Andrew Harvey:
> On 6/5/15, 3:46 AM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:
> >Stephen and me say the same thing about using the ethdev API.
> 
> And your would have a point would be valid if dpdk were available to every
> interface we support (it is not) and on every processor architecture that
> we support (it is not) and every OS we support (it is not).  So to
> minimize entropy in the code why not leave the client code the same
> ioctl(fd, …) and hide the implementation
> detail in a wrapper library.

Please, explain the relation between an ioctl and the DPDK.
  
Thomas Monjalon June 5, 2015, 9:03 p.m. UTC | #22
2015-06-05 17:24, Andrew Harvey:
> On 6/5/15, 5:47 AM, "Bruce Richardson" <bruce.richardson@intel.com> wrote:
> >> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> >> > That's why we need to understand what we (or you) are missing.
> >> > Maybe that it would be clearer with some code examples (which would
> >> > go in the lib documentation if any).
> >> > 
> >> > Thanks
> >
> >How about doing this work as a sample application initially, to
> >demonstrate how
> >an application written using ethtool APIs could be shimmed to use DPDK
> >underneath.
> >The ethtool to dpdk mapping could be contained in a single header file
> >(or header
> >and c file) inside the sample app. This would allow easy re-use of the
> >shim
> >layer, while at the same time not making it part of the core DPDK
> >libraries.
> >
> >Regards,
> >/Bruce
> 
> This would appear to be the most pragmatic way forward.  It would allow
> others to see more of the code and judge its value for themselves. I have
> no issues with this approach if others agree.

Since the beginning of this thread, a doc is requested (many times) to
show the benefit of integrating such a layer.
If you prefer coding a full example, it would probably also be fine.
  

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 9362c19..b8b481f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -186,6 +186,10 @@  M: Thomas Monjalon <thomas.monjalon@6wind.com>
 F: lib/librte_ether/
 F: scripts/test-null.sh
 
+Ethtool API
+M: Liang-Min Larry Wang <liang-min.wang@intel.com>
+F: lib/librte_ethtool/
+
 
 Drivers
 -------
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 0078dc9..f5759fd 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -129,6 +129,11 @@  CONFIG_RTE_LIBRTE_EAL_VMWARE_TSC_MAP_SUPPORT=y
 CONFIG_RTE_LIBRTE_KVARGS=y
 
 #
+# Compile user-space ethtool library
+#
+CONFIG_RTE_LIBRTE_ETHTOOL=y
+
+#
 # Compile generic ethernet library
 #
 CONFIG_RTE_LIBRTE_ETHER=y
diff --git a/lib/Makefile b/lib/Makefile
index 5f480f9..a6c7375 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -41,6 +41,7 @@  DIRS-$(CONFIG_RTE_LIBRTE_TIMER) += librte_timer
 DIRS-$(CONFIG_RTE_LIBRTE_CFGFILE) += librte_cfgfile
 DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += librte_cmdline
 DIRS-$(CONFIG_RTE_LIBRTE_ETHER) += librte_ether
+DIRS-$(CONFIG_RTE_LIBRTE_ETHTOOL) += librte_ethtool
 DIRS-$(CONFIG_RTE_LIBRTE_VHOST) += librte_vhost
 DIRS-$(CONFIG_RTE_LIBRTE_HASH) += librte_hash
 DIRS-$(CONFIG_RTE_LIBRTE_LPM) += librte_lpm
diff --git a/lib/librte_ethtool/Makefile b/lib/librte_ethtool/Makefile
new file mode 100644
index 0000000..1d981f6
--- /dev/null
+++ b/lib/librte_ethtool/Makefile
@@ -0,0 +1,56 @@ 
+#   BSD LICENSE
+#
+#   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+#   All rights reserved.
+#
+#   Redistribution and use in source and binary forms, with or without
+#   modification, are permitted provided that the following conditions
+#   are met:
+#
+#     * Redistributions of source code must retain the above copyright
+#       notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above copyright
+#       notice, this list of conditions and the following disclaimer in
+#       the documentation and/or other materials provided with the
+#       distribution.
+#     * Neither the name of Intel Corporation nor the names of its
+#       contributors may be used to endorse or promote products derived
+#       from this software without specific prior written permission.
+#
+#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+include $(RTE_SDK)/mk/rte.vars.mk
+
+#
+# library name
+#
+LIB = librte_ethtool.a
+
+CFLAGS += -O3
+CFLAGS += $(WERROR_FLAGS)
+
+EXPORT_MAP := rte_ethtool_version.map
+
+LIBABIVER := 1
+
+SRCS-y += rte_ethtool.c
+
+#
+# Export include files
+#
+SYMLINK-y-include += rte_ethtool.h
+
+# this lib depends upon:
+DEPDIRS-y += lib/librte_ether
+
+include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/lib/librte_ethtool/rte_ethtool.c b/lib/librte_ethtool/rte_ethtool.c
new file mode 100644
index 0000000..2ccf06f
--- /dev/null
+++ b/lib/librte_ethtool/rte_ethtool.c
@@ -0,0 +1,155 @@ 
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <rte_version.h>
+#include <rte_ethdev.h>
+#include "rte_ethtool.h"
+
+int
+rte_ethtool_get_drvinfo(uint8_t port_id, struct ethtool_drvinfo *drvinfo)
+{
+	struct rte_eth_dev_info dev_info;
+
+	memset(&dev_info, 0, sizeof(dev_info));
+	rte_eth_dev_info_get(port_id, &dev_info);
+
+	snprintf(drvinfo->driver, sizeof(drvinfo->driver), "%s",
+		dev_info.driver_name);
+	snprintf(drvinfo->version, sizeof(drvinfo->version), "%s",
+		rte_version());
+	snprintf(drvinfo->bus_info, sizeof(drvinfo->bus_info),
+		"%04x:%02x:%02x.%x",
+		dev_info.pci_dev->addr.domain, dev_info.pci_dev->addr.bus,
+		dev_info.pci_dev->addr.devid, dev_info.pci_dev->addr.function);
+
+	drvinfo->n_stats = sizeof(struct rte_eth_stats) / sizeof(uint64_t);
+	drvinfo->testinfo_len = 0;
+
+	return 0;
+}
+
+int
+rte_ethtool_get_link(uint8_t port_id)
+{
+	struct rte_eth_link link;
+
+	rte_eth_link_get(port_id, &link);
+	return link.link_status;
+}
+
+int
+rte_ethtool_net_open(uint8_t port_id)
+{
+	rte_eth_dev_stop(port_id);
+
+	return rte_eth_dev_start(port_id);
+}
+
+int
+rte_ethtool_net_stop(uint8_t port_id)
+{
+	rte_eth_dev_stop(port_id);
+
+	return 0;
+}
+
+int
+rte_ethtool_net_get_mac_addr(uint8_t port_id, struct ether_addr *addr)
+{
+	rte_eth_macaddr_get(port_id, addr);
+
+	return 0;
+}
+
+int
+rte_ethtool_net_set_mac_addr(uint8_t port_id, struct ether_addr *addr)
+{
+	return rte_eth_dev_default_mac_addr_set(port_id, addr);
+}
+
+int
+rte_ethtool_net_validate_addr(uint8_t port_id __rte_unused,
+	struct ether_addr *addr)
+{
+	return is_valid_assigned_ether_addr(addr);
+}
+
+int
+rte_ethtool_net_set_config(uint8_t port_id, void *config __rte_unused)
+{
+	struct rte_eth_link link;
+
+	memset(&link, 0, sizeof(link));
+	rte_eth_link_get(port_id, &link);
+	if (link.link_status == 1)
+		return -EINVAL;
+	return 0;
+}
+
+int
+rte_ethtool_net_change_mtu(uint8_t port_id, int mtu)
+{
+	return rte_eth_dev_set_mtu(port_id, (uint16_t)mtu);
+}
+
+int
+rte_ethtool_net_get_stats64(uint8_t port_id, struct rte_eth_stats *stats)
+{
+	return rte_eth_stats_get(port_id, stats);
+}
+
+int
+rte_ethtool_net_vlan_rx_add_vid(uint8_t port_id, uint16_t vid)
+{
+	return rte_eth_dev_vlan_filter(port_id, vid, 1);
+}
+
+int
+rte_ethtool_net_vlan_rx_kill_vid(uint8_t port_id, uint16_t vid)
+{
+	return rte_eth_dev_vlan_filter(port_id, vid, 0);
+}
+
+int
+rte_ethtool_net_set_rx_mode(uint8_t port_id __rte_unused)
+{
+	/*
+	 * The set_rx_mode op is part of pmd driver start operation, and
+	 * the ethdev api maintains software configuration parameters and under-
+	 * line hardware states consistent, so no operation is needed for
+	 * rte_ethtool_net_set_rx_mode().
+	 */
+	return 0;
+}
diff --git a/lib/librte_ethtool/rte_ethtool.h b/lib/librte_ethtool/rte_ethtool.h
new file mode 100644
index 0000000..cb68d94
--- /dev/null
+++ b/lib/librte_ethtool/rte_ethtool.h
@@ -0,0 +1,257 @@ 
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef _RTE_ETHTOOL_H_
+#define _RTE_ETHTOOL_H_
+
+/*
+ * This new interface is designed to provide a user-space shim layer for
+ * Ethtool and Netdevice op API.
+ *
+ * rte_ethtool_get_driver:          ethtool_ops::get_driverinfo
+ * rte_ethtool_get_link:            ethtool_ops::get_link
+ *
+ * rte_ethtool_net_open:            net_device_ops::ndo_open
+ * rte_ethtool_net_stop:            net_device_ops::ndo_stop
+ * rte_ethtool_net_set_mac_addr:    net_device_ops::ndo_set_mac_address
+ * rte_ethtool_net_validate_addr:   net_device_ops::ndo_validate_addr
+ * rte_ethtool_net_set_config:      net_device_ops::ndo_set_config
+ * rte_ethtool_net_change_mtu:      net_device_ops::ndo_net_change_mtu
+ * rte_ethtool_net_get_stats64:     net_device_ops::ndo_get_stats64
+ * rte_ethtool_net_vlan_rx_add_vid  net_device_ops::ndo_vlan_rx_add_vid
+ * rte_ethtool_net_vlan_rx_kill_vid net_device_ops::ndo_vlan_rx_kill_vid
+ * rte_ethtool_net_set_rx_mode      net_device_ops::ndo_set_rx_mode
+ *
+ */
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdint.h>
+#include <rte_ethdev.h>
+#include <linux/ethtool.h>
+
+/**
+ * Retrieve the Ethernet device driver information according to attributes described by
+ * ethtool data structure, ethtool_drvinfo
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param drvinfo
+ *   A pointer to get driver information
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_get_drvinfo(uint8_t port_id, struct ethtool_drvinfo *drvinfo);
+
+/**
+ * Retrieve the Ethernet device link status
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @return
+ *   - (1) if link up.
+ *   - (0) if link down.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_get_link(uint8_t port_id);
+
+/**
+ * Start the Ethernet device.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_open(uint8_t port_id);
+
+/**
+ * Stop the Ethernet device.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_stop(uint8_t port_id);
+
+/**
+ * Get the Ethernet device MAC address.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param addr
+ *	 MAC address of the Ethernet device.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_get_mac_addr(uint8_t port_id, struct ether_addr *addr);
+
+/**
+ * Setting the Ethernet device MAC address.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param addr
+ *	 The new MAC addr.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_set_mac_addr(uint8_t port_id, struct ether_addr *addr);
+
+/**
+ * Validate if the provided MAC address is valid unicast address
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param addr
+ *	 A pointer to a buffer (6-byte, 48bit) for the target MAC address
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_validate_addr(uint8_t port_id, struct ether_addr *addr);
+
+/**
+ * Setting the Ethernet device configuration.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param config
+ *	 A opintr to a configuration parameter.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_set_config(uint8_t port_id, void *config);
+
+/**
+ * Setting the Ethernet device maximum Tx unit.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param mtu
+ *	 New MTU
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu);
+
+/**
+ * Retrieve the Ethernet device traffic statistics
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param stats
+ *	 A pointer to struct rte_eth_stats for statistics parameters
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_get_stats64(uint8_t port_id, struct rte_eth_stats *stats);
+
+/**
+ * Update the Ethernet device VLAN filter with new vid
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param vid
+ *	 A new VLAN id
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_vlan_rx_add_vid(uint8_t port_id, uint16_t vid);
+
+/**
+ * Remove VLAN id from Ethernet device.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param vid
+ *	 A new VLAN id
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_vlan_rx_kill_vid(uint8_t port_id, uint16_t vid);
+
+/**
+ * Setting the Ethernet device rx mode.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - others depends on the specific operations implementation.
+ */
+int rte_ethtool_net_set_rx_mode(uint8_t port_id);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_ETHTOOL_H_ */
diff --git a/lib/librte_ethtool/rte_ethtool_version.map b/lib/librte_ethtool/rte_ethtool_version.map
new file mode 100644
index 0000000..82fc0d3
--- /dev/null
+++ b/lib/librte_ethtool/rte_ethtool_version.map
@@ -0,0 +1,18 @@ 
+DPDK_2.0 {
+	global:
+
+	rte_ethtool_net_open;
+	rte_ethtool_net_stop;
+	rte_ethtool_net_get_mac_addr;
+	rte_ethtool_net_get_mac_addr;
+	rte_ethtool_net_validate_addr;
+	rte_ethtool_net_set_config;
+	rte_ethtool_net_change_mtu;
+	rte_ethtool_net_get_stats64;
+	rte_ethtool_net_vlan_rx_add_vid;
+	rte_ethtool_net_vlan_rx_kill_vid;
+	rte_ethtool_get_drvinfo;
+	rte_ethtool_get_link;
+
+	local: *;
+};
diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index 1a2043a..86867a6 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -105,6 +105,7 @@  ifeq ($(CONFIG_RTE_BUILD_COMBINE_LIBS),n)
 _LDLIBS-$(CONFIG_RTE_LIBRTE_KVARGS)         += -lrte_kvargs
 _LDLIBS-$(CONFIG_RTE_LIBRTE_MBUF)           += -lrte_mbuf
 _LDLIBS-$(CONFIG_RTE_LIBRTE_IP_FRAG)        += -lrte_ip_frag
+_LDLIBS-$(CONFIG_RTE_LIBRTE_ETHTOOL)        += -lrte_ethtool
 _LDLIBS-$(CONFIG_RTE_LIBRTE_ETHER)          += -lethdev
 _LDLIBS-$(CONFIG_RTE_LIBRTE_MALLOC)         += -lrte_malloc
 _LDLIBS-$(CONFIG_RTE_LIBRTE_MEMPOOL)        += -lrte_mempool