From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <liang-min.wang@intel.com> Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id C43ECC332 for <dev@dpdk.org>; Tue, 2 Jun 2015 17:47:44 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 02 Jun 2015 08:47:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,540,1427785200"; d="scan'208";a="704014402" Received: from orsmsx104.amr.corp.intel.com ([10.22.225.131]) by orsmga001.jf.intel.com with ESMTP; 02 Jun 2015 08:47:43 -0700 Received: from orsmsx116.amr.corp.intel.com (10.22.240.14) by ORSMSX104.amr.corp.intel.com (10.22.225.131) with Microsoft SMTP Server (TLS) id 14.3.224.2; Tue, 2 Jun 2015 08:47:42 -0700 Received: from fmsmsx151.amr.corp.intel.com (10.18.125.4) by ORSMSX116.amr.corp.intel.com (10.22.240.14) with Microsoft SMTP Server (TLS) id 14.3.224.2; Tue, 2 Jun 2015 08:47:42 -0700 Received: from FMSMSX110.amr.corp.intel.com ([169.254.14.46]) by FMSMSX151.amr.corp.intel.com ([169.254.7.230]) with mapi id 14.03.0224.002; Tue, 2 Jun 2015 08:47:42 -0700 From: "Wang, Liang-min" <liang-min.wang@intel.com> To: Thomas Monjalon <thomas.monjalon@6wind.com> Thread-Topic: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs Thread-Index: AQHQmkV0vh50QuHJo0WVOZnDigauCp2ZosAA//+RahCAAI6iAP//nQiA Date: Tue, 2 Jun 2015 15:47:41 +0000 Message-ID: <B6CB929FEBC10D4FAC4BCA7EF2298E2571765AC2@FMSMSX110.amr.corp.intel.com> References: <1432927612-12244-1-git-send-email-liang-min.wang@intel.com> <2827467.MLJnnY93Dx@xps13> <B6CB929FEBC10D4FAC4BCA7EF2298E2571765939@FMSMSX110.amr.corp.intel.com> <3076202.B6CvAKP4DR@xps13> In-Reply-To: <3076202.B6CvAKP4DR@xps13> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.1.200.107] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" <dev@dpdk.org> Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Tue, 02 Jun 2015 15:47:45 -0000 -----Original Message----- From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]=20 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 eth= tool-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=20 > >>> bear the same functionality as ethtool_ops (linux/ethtool.h) and=20 > >>> net_device_ops (linux/netdevice.h). > >>>=20 > >>> 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 >>=20 >> Since ethtool is provided to assist users migrating from kernel ethtool/= net_device_op based design to user-space DPDK device management. The ethtoo= l API's are created to closely maintain its original interface, therefore t= his library depends on <linux/ethool.h>. To avoid pollute the existing ethd= ev interface, a new library is created. To minimize code replication and ma= intain 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. >>=20 > >>- There is no obvious interest (how is it supposed to be used?) >> There are already two acknowledge on this release. Earlier comment on th= is 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 n= ever 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 d= pdk 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 co= mment ...) or the question on putting ethtool on separate library. For the = latter concern, as described, the design is to avoid polluting ethdev libra= ry 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 r= elease first. > >>Other comments: > >>- the patches are not versioned >>=20 >> There is version file. Not sure what do you mean "the patches are not ve= rsioned" >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 >>=20 >> Will fix that. >>=20 > >>I'm curious to understand how renaming rte_eth_dev_set_mtu() to > >>rte_ethtool_net_change_mtu() will help anyone. >>=20 >> As described, this interface is designed to provide API closely to kerne= l 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 e= xisting rte_eth_dev_set_mtu. I don't see the benefit. The benefit is single interface for users to access. Instead of looking int= o two different interfaces (ethtool, ether).=20