From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id AD46D2BA2 for ; Wed, 9 Mar 2016 14:36:52 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga103.jf.intel.com with ESMTP; 09 Mar 2016 05:36:51 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,311,1455004800"; d="scan'208";a="933023458" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by fmsmga002.fm.intel.com with ESMTP; 09 Mar 2016 05:36:50 -0800 Received: from irsmsx111.ger.corp.intel.com (10.108.20.4) by IRSMSX101.ger.corp.intel.com (163.33.3.153) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 9 Mar 2016 13:36:49 +0000 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.35]) by irsmsx111.ger.corp.intel.com ([169.254.2.127]) with mapi id 14.03.0248.002; Wed, 9 Mar 2016 13:36:49 +0000 From: "Ananyev, Konstantin" To: Thomas Monjalon , "Kulasek, TomaszX" Thread-Topic: [dpdk-dev] [PATCH v2 1/2] ethdev: add buffered tx api Thread-Index: AQHRbyZIC6RAW0jzIEeJRoaRUBOzep9QO/iAgADx3hA= Date: Wed, 9 Mar 2016 13:36:49 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836B1A4A2@irsmsx105.ger.corp.intel.com> References: <1452869038-9140-1-git-send-email-tomaszx.kulasek@intel.com> <1456333729-3804-1-git-send-email-tomaszx.kulasek@intel.com> <1456333729-3804-2-git-send-email-tomaszx.kulasek@intel.com> <4446137.g0qt5Fe5Df@xps13> In-Reply-To: <4446137.g0qt5Fe5Df@xps13> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjAwNmFlNmEtZTE2Ny00ZmY1LWI5OWEtYzk4MDcxZTczMjVlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6ImpuOW9kc0hibHMyT3pWZDd1ZWYyRzNOWTE1UVNPZ2tEWGRuWEt4WUM5SU09In0= x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2 1/2] ethdev: add buffered tx api X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Mar 2016 13:36:55 -0000 Hi Thomas, >=20 > Hi, >=20 > It is an overlay on the tx burst API. > Probably it doesn't hurt to add it but we have to be really cautious > with the API definition to try keeping it stable in the future. >=20 > 2016-02-24 18:08, Tomasz Kulasek: > > +/** > > + * Structure used to buffer packets for future TX > > + * Used by APIs rte_eth_tx_buffer and rte_eth_tx_buffer_flush > > + */ > > +struct rte_eth_dev_tx_buffer { > > + unsigned nb_pkts; >=20 > What about "length"? > Why is it unsigned and the size is uint16_t? Good point, yes need to make it consistent. >=20 > > + uint64_t errors; > > + /**< Total number of queue packets to sent that are dropped. */ >=20 > The errors are passed as userdata to the default callback. > If we really want to have this kind of counter, we can define our > own callback. So why defining this field as standard? > I would like to keep it as simple as possible. >=20 > > + buffer_tx_error_fn cbfn; >=20 > Why not simply "callback" as name? >=20 > > + void *userdata; > > + uint16_t size; /**< Size of buffer for buffered tx */ > > + struct rte_mbuf *pkts[]; > > +}; >=20 > What is the benefit of exposing this structure in the API, > except that it is used in some inline functions? >=20 I described the benefits, I think it provides here: http://dpdk.org/ml/archives/dev/2016-February/033058.html > > +static inline uint16_t > > +rte_eth_tx_buffer_flush(uint8_t port_id, uint16_t queue_id, > > + struct rte_eth_dev_tx_buffer *buffer) > > +{ > > + uint16_t sent; > > + > > + uint16_t to_send =3D buffer->nb_pkts; > > + > > + if (to_send =3D=3D 0) > > + return 0; >=20 > Why this check is done in the lib? > What is the performance gain if we are idle? > It can be done outside if needed. Yes, that could be done outside, but if user has to do it anyway, why not to put it inside? I don't expect any performance gain/loss because of that - just seems a bit more convenient to the user. Konstantin >=20 > > + sent =3D rte_eth_tx_burst(port_id, queue_id, buffer->pkts, to_send); > > + > > + buffer->nb_pkts =3D 0; > > + > > + /* All packets sent, or to be dealt with by callback below */ > > + if (unlikely(sent !=3D to_send)) > > + buffer->cbfn(&buffer->pkts[sent], to_send - sent, > > + buffer->userdata); > > + > > + return sent; > > +} > [...] > > +/** > > + * Callback function for tracking unsent buffered packets. > > + * > > + * This function can be passed to rte_eth_tx_buffer_set_err_callback()= to > > + * adjust the default behaviour when buffered packets cannot be sent. = This > > + * function drops any unsent packets, but also updates a user-supplied= counter > > + * to track the overall number of packets dropped. The counter should = be an > > + * uint64_t variable. > > + * > > + * NOTE: this function should not be called directly, instead it shoul= d be used > > + * as a callback for packet buffering. > > + * > > + * NOTE: when configuring this function as a callback with > > + * rte_eth_tx_buffer_set_err_callback(), the final, userdata par= ameter > > + * should point to an uint64_t value. >=20 > Please forget this idea of counter in the default callback. >=20 > [...] > > +void > > +rte_eth_count_unsent_packet_callback(struct rte_mbuf **pkts, uint16_t = unsent, > > + void *userdata); >=20 > What about rte_eth_tx_buffer_default_callback as name?