From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id BE72A1BBBA for ; Thu, 10 May 2018 21:48:04 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 May 2018 12:48:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,386,1520924400"; d="scan'208";a="227521798" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.252.25.211]) ([10.252.25.211]) by fmsmga006.fm.intel.com with ESMTP; 10 May 2018 12:48:00 -0700 To: Andrew Rybchenko , Wei Dai , thomas@monjalon.net Cc: dev@dpdk.org, Qi Zhang References: <1525913371-18178-1-git-send-email-wei.dai@intel.com> <1525913806-41723-1-git-send-email-wei.dai@intel.com> From: Ferruh Yigit Openpgp: preference=signencrypt Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= xsFNBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABzSVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+wsF+BBMBAgAoAhsDBgsJCAcDAgYVCAIJCgsE FgIDAQIeAQIXgAUCWZR3VQUJB33WBQAKCRD5M+tD3xNhH6DWEACVhEb8q1epPwZrUDoxzu7E TS1b8tmabOmnjXZRs6+EXgUVHkp2xxkCfDmL3pa5bC0G/74aJnWjNsdvE05V1cb4YK4kRQ62 FwDQ+hlrFrwFB3PtDZk1tpkzCRHvJgnIil+0MuEh32Y57ig6hy8yO8ql7Lohyrnpfk/nNpm4 jQGEF5qEeHcEFe1AZQlPHN/STno8NZSz2nl0b2cw+cujN1krmvB52Ah/2KugQ6pprVyrGrzB c34ZQO9OsmSjJlETCZk6EZzuhfe16iqBFbOSadi9sPcJRwaUQBid+xdFWl7GQ8qC3zNPibSF HmU43yBZUqJDZlhIcl6/cFpOSjv2sDWdtjEXTDn5y/0FsuY0mFE78ItC4kCTIVk17VZoywcd fmbbnwOSWzDq7hiUYuQGkIudJw5k/A1CMsyLkoUEGN3sLfsw6KASgS4XrrmPO4UVr3mH5bP1 yC7i1OVNpzvOxtahmzm481ID8sk72GC2RktTOHb0cX+qdoiMMfYgo3wRRDYCBt6YoGYUxF1p msjocXyqToKhhnFbXLaZlVfnQ9i2i8jsj9SKig+ewC2p3lkPj6ncye9q95bzhmUeJO6sFhJg Hiz6syOMg8yCcq60j07airybAuHIDNFWk0gaWAmtHZxLObZx2PVn2nv9kLYGohFekw0AOsIW ta++5m48dnCoAc7BTQRX1ky+ARAApzQNvXvE2q1LAS+Z+ni2R13Bb1cDS1ZYq1jgpR13+OKN ipzd8MPngRJilXxBaPTErhgzR0vGcNTYhjGMSyFIHVOoBq1VbP1a0Fi/NqWzJOowo/fDfgVy K4vuitc/gCJs+2se4hdZA4EQJxVlNM51lgYDNpjPGIA43MX15OLAip73+ho6NPBMuc5qse3X pAClNhBKfENRCWN428pi3WVkT+ABRTE0taxjJNP7bb+9TQYNRqGwnGzX5/XISv44asWIQCaq vOkXSUJLd//cdVNTqtL1wreCVVR5pMXj7VIrlk07fmmJVALCmGbFr53BMb8O+8dgK2A5mitM n44d+8KdJWOwziRxcaMk/LclmZS3Iv1TERtiWt98Y9AjeAtcgYPkA3ld0BcUKONogP8pHVz1 Ed3s5rDQ91yr1S0wuAzW91fxGUO4wY+uPmxCtFVuBgd9VT9NAKTUL0qHM7CDgCnZPe0TW6Zj 8OqtdCCyAfvU9cW5xWM7Icxhde6AtPxhDSBwE8fL2ZmrDmaA4jmUKXp3i4JxRPSX84S08b+s DWXHPxy10UFU5A7EK/BEbZAKBwn9ROfm+WK+6X5xOGLoRE++OqNuUudxC1GDyLOPaqCbBCS9 +P6HsTHzxsjyJa27n4jcrcuY3P9TEcFJYSZSeSDh8mVGvugi0exnSJrrBZDyVCcAEQEAAcLB ZQQYAQIADwIbDAUCWZR1ZwUJA59cIQAKCRD5M+tD3xNhH5b+D/9XG44Ci6STdcA5RO/ur05J EE3Ux1DCHZ5V7vNAtX/8Wg4l4GZfweauXwuJ1w7Sp7fklwcNC6wsceI+EmNjGMqfIaukGetG +jBGqsQ7moOZodfXUoCK98gblKgt/BPYMVidzlGC8Q/+lZg1+o29sPnwImW+MXt/Z5az/Z17 Qc265g+p5cqJHzq6bpQdnF7Fu6btKU/kv6wJghENvgMXBuyThqsyFReJWFh2wfaKyuix3Zyj ccq7/blkhzIKmtFWgDcgaSc2UAuJU+x9nuYjihW6WobpKP/nlUDu3BIsbIq09UEke+uE/QK+ FJ8PTJkAsXOf1Bc2C0XbW4Y2hf103+YY6L8weUCBsWC5VH5VtVmeuh26ENURclwfeXhWQ9Og 77yzpTXWr5g1Z0oLpYpWPv745J4bE7pv+dzxOrFdM1xNkzY2pvXph/A8OjxZNQklDkHQ7PIB Lki5L2F4XkEOddUUQchJwzMqTPsggPDmGjgLZrqgO+s4ECZK5+nLD3HEpAbPa3JLDaScy+90 Nu1lAqPUHSnP3vYZVw85ZYm6UCxHE4VLMnnJsN09ZhsOSVR+GyP5Nyw9rT1V3lcsuH7M5Naa 2Xobn9m7l9bRCD/Ji8kG15eV1WTxx1HXVQGjdUYDI7UwegBNbwMLh17XDy+3sn/6SgcqtECA Q6pZKA2mTQxEKMLBZQQYAQIADwIbDAUCWZR3hQUJA59eRwAKCRD5M+tD3xNhH4a/D/4jLAZu UhvU1swWcNEVVCELZ0D3LOV14XcY2MXa3QOpeZ9Bgq7YYJ4S5YXK+SBQS0FkRZdjGNvlGZoG ZdpU+NsQmQFhqHGwX0IT9MeTFM8uvKgxNKGwMVcV9g0IOqwBhGHne+BFboRA9362fgGW5AYQ zT0mzzRKEoOh4r3AQvbM6kLISxo0k1ujdYiI5nj/5WoKDqxTwwfuN1uDUHsWo3tzenRmpMyU NyW3Dc+1ajvXLyo09sRRq7BnM99Rix1EGL8Qhwy+j0YAv+FuspWxUX9FxXYho5PvGLHLsHfK FYQ7x/RRbpMjkJWVfIe/xVnfvn4kz+MTA5yhvsuNi678fLwY9hBP0y4lO8Ob2IhEPdfnTuIs tFVxXuelJ9xAe5TyqP0f+fQjf1ixsBZkqOohsBXDfje0iaUpYa/OQ/BBeej0dUdg2JEu4jAC x41HpVCnP9ipLpD0fYz1d/dX0F/VY2ovW6Eba/y/ngOSAR6C+u881m7oH2l0G47MTwkaQCBA bLGXPj4TCdX3lftqt4bcBPBJ+rFAnJmRHtUuyyaewBnZ81ZU2YAptqFM1kTh+aSvMvGhfVsQ qZL2rk2OPN1hg+KXhErlbTZ6oPtLCFhSHQmuxQ4oc4U147wBTUuOdwNjtnNatUhRCp8POc+3 XphVR5G70mnca1E2vzC77z+XSlTyRA== Message-ID: <69b2172d-0629-bf95-fdcc-e9b77b98866e@intel.com> Date: Thu, 10 May 2018 20:47:59 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v10] ethdev: new Rx/Tx offloads API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 May 2018 19:48:05 -0000 On 5/10/2018 10:25 AM, Andrew Rybchenko wrote: > On 05/10/2018 03:56 AM, Wei Dai wrote: >> This patch check if a input requested offloading is valid or not. >> Any reuqested offloading must be supported in the device capabilities. >> Any offloading is disabled by default if it is not set in the parameter >> dev_conf->[rt]xmode.offloads to rte_eth_dev_configure( ) and >> [rt]x_conf->offloads to rte_eth_[rt]x_queue_setup( ). >> If any offloading is enabled in rte_eth_dev_configure( ) by application, >> it is enabled on all queues no matter whether it is per-queue or >> per-port type and no matter whether it is set or cleared in >> [rt]x_conf->offloads to rte_eth_[rt]x_queue_setup( ). >> If a per-queue offloading hasn't be enabled in rte_eth_dev_configure( ), >> it can be enabled or disabled for individual queue in >> ret_eth_[rt]x_queue_setup( ). >> A new added offloading is the one which hasn't been enabled in >> rte_eth_dev_configure( ) and is reuqested to be enabled in >> rte_eth_[rt]x_queue_setup( ), it must be per-queue type, >> otherwise triger an error log. >> The underlying PMD must be aware that the requested offloadings >> to PMD specific queue_setup( ) function only carries those >> new added offloadings of per-queue type. >> >> This patch can make above such checking in a common way in rte_ethdev >> layer to avoid same checking in underlying PMD. >> >> This patch assumes that all PMDs in 18.05-rc2 have already >> converted to offload API defined in 17.11 . It also assumes >> that all PMDs can return correct offloading capabilities >> in rte_eth_dev_infos_get( ). >> >> In the beginning of [rt]x_queue_setup( ) of underlying PMD, >> add offloads = [rt]xconf->offloads | >> dev->data->dev_conf.[rt]xmode.offloads; to keep same as offload API >> defined in 17.11 to avoid upper application broken due to offload >> API change. >> PMD can use the info that input [rt]xconf->offloads only carry >> the new added per-queue offloads to do some optimization or some >> code change on base of this patch. >> >> Signed-off-by: Wei Dai >> Signed-off-by: Ferruh Yigit >> Signed-off-by: Qi Zhang >> >> --- >> v10: >> sorry, miss the code change, fix the buidling error >> >> v9: >> replace RTE_PMD_DEBUG_TRACE with ethdev_log(ERR, in ethdev >> to avoid failure of application which hasn't been completely >> converted to new offload API. > > [...] > >> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c >> index e560524..5baa2aa 100644 >> --- a/lib/librte_ethdev/rte_ethdev.c >> +++ b/lib/librte_ethdev/rte_ethdev.c >> @@ -1139,6 +1139,28 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, >> ETHER_MAX_LEN; >> } >> >> + /* Any requested offloading must be within its device capabilities */ >> + if ((local_conf.rxmode.offloads & dev_info.rx_offload_capa) != >> + local_conf.rxmode.offloads) { >> + ethdev_log(ERR, "ethdev port_id=%d requested Rx offloads " >> + "0x%" PRIx64 " doesn't match Rx offloads " >> + "capabilities 0x%" PRIx64 " in %s( )\n", >> + port_id, >> + local_conf.rxmode.offloads, >> + dev_info.rx_offload_capa, >> + __func__); > > Why is return -EINVAL removed here? > If application is not updated to use offloads, offloads is 0 and everything is OK. > If application is updated to use offloads, its behaviour must be consistent. > Same below for Tx device offloads. To be cautious to not break apps for the cases we have missed. For example testpmd was giving error with virtual PMDs because of CRC_STRIP, it is easy to fix testpmd but other applications too may have similar problem. Overall agree that error should be return, for this release it will only print error log, next release we can add return back. Next release applications will be switched to new offloading API so they will already need to be updated, and hopefully that change will be before rc stage. > >> + } >> + if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) != >> + local_conf.txmode.offloads) { >> + ethdev_log(ERR, "ethdev port_id=%d requested Tx offloads " >> + "0x%" PRIx64 " doesn't match Tx offloads " >> + "capabilities 0x%" PRIx64 " in %s( )\n", >> + port_id, >> + local_conf.txmode.offloads, >> + dev_info.tx_offload_capa, >> + __func__); >> + } >> + >> /* Check that device supports requested rss hash functions. */ >> if ((dev_info.flow_type_rss_offloads | >> dev_conf->rx_adv_conf.rss_conf.rss_hf) != >> @@ -1504,6 +1526,38 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id, >> &local_conf.offloads); >> } >> >> + /* >> + * If an offloading has already been enabled in >> + * rte_eth_dev_configure(), it has been enabled on all queues, >> + * so there is no need to enable it in this queue again. >> + * The local_conf.offloads input to underlying PMD only carries >> + * those offloadings which are only enabled on this queue and >> + * not enabled on all queues. >> + * The underlying PMD must be aware of this point. >> + */ >> + local_conf.offloads &= ~dev->data->dev_conf.rxmode.offloads; >> + >> + /* >> + * New added offloadings for this queue are those not enabled in >> + * rte_eth_dev_configure( ) and they must be per-queue type. >> + * A pure per-port offloading can't be enabled on a queue while >> + * disabled on another queue. A pure per-port offloading can't >> + * be enabled for any queue as new added one if it hasn't been >> + * enabled in rte_eth_dev_configure( ). >> + */ >> + if ((local_conf.offloads & dev_info.rx_queue_offload_capa) != >> + local_conf.offloads) { >> + ethdev_log(ERR, "Ethdev port_id=%d rx_queue_id=%d, new " >> + "added offloads 0x%" PRIx64 " must be " >> + "within pre-queue offload capabilities 0x%" >> + PRIx64 " in %s( )\n", >> + port_id, >> + rx_queue_id, >> + local_conf.offloads, >> + dev_info.rx_queue_offload_capa, >> + __func__); > > May be it is really a good tradeoff to remove error return here. > Ideally it would be nice to see explanation here why. > >> + } >> + >> ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc, >> socket_id, &local_conf, mp); >> if (!ret) { >> @@ -1612,6 +1666,38 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id, >> &local_conf.offloads); >> } >> >> + /* >> + * If an offloading has already been enabled in >> + * rte_eth_dev_configure(), it has been enabled on all queues, >> + * so there is no need to enable it in this queue again. >> + * The local_conf.offloads input to underlying PMD only carries >> + * those offloadings which are only enabled on this queue and >> + * not enabled on all queues. >> + * The underlying PMD must be aware of this point. >> + */ >> + local_conf.offloads &= ~dev->data->dev_conf.txmode.offloads; >> + >> + /* >> + * New added offloadings for this queue are those not enabled in >> + * rte_eth_dev_configure( ) and they must be per-queue type. >> + * A pure per-port offloading can't be enabled on a queue while >> + * disabled on another queue. A pure per-port offloading can't >> + * be enabled for any queue as new added one if it hasn't been >> + * enabled in rte_eth_dev_configure( ). >> + */ >> + if ((local_conf.offloads & dev_info.tx_queue_offload_capa) != >> + local_conf.offloads) { >> + ethdev_log(ERR, "Ethdev port_id=%d tx_queue_id=%d, new " >> + "added offloads 0x%" PRIx64 " must be " >> + "within pre-queue offload capabilities 0x%" >> + PRIx64 " in %s( )\n", >> + port_id, >> + tx_queue_id, >> + local_conf.offloads, >> + dev_info.tx_queue_offload_capa, >> + __func__); >> + } >> + >> return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev, >> tx_queue_id, nb_tx_desc, socket_id, &local_conf)); >> } >