From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <bruce.richardson@intel.com> Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 1D55C532D for <dev@dpdk.org>; Mon, 18 Sep 2017 13:05:01 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga105.jf.intel.com with ESMTP; 18 Sep 2017 04:05:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,412,1500966000"; d="scan'208";a="901296075" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.24]) by FMSMGA003.fm.intel.com with SMTP; 18 Sep 2017 04:04:58 -0700 Received: by (sSMTP sendmail emulation); Mon, 18 Sep 2017 12:04:57 +0100 Date: Mon, 18 Sep 2017 12:04:56 +0100 From: Bruce Richardson <bruce.richardson@intel.com> To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com> Cc: Thomas Monjalon <thomas@monjalon.net>, "stephen@networkplumber.org" <stephen@networkplumber.org>, "dev@dpdk.org" <dev@dpdk.org>, Shahaf Shuler <shahafs@mellanox.com> Message-ID: <20170918110456.GB15516@bricha3-MOBL3.ger.corp.intel.com> References: <cover.1504508374.git.shahafs@mellanox.com> <12544923.ZPp1eIik3W@xps> <2601191342CEEE43887BDE71AB9772584F24AA9D@irsmsx105.ger.corp.intel.com> <6120098.SpXJOblVKo@xps> <20170918103154.GA13172@bricha3-MOBL3.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772584F24BFBF@irsmsx105.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB9772584F24BFBF@irsmsx105.ger.corp.intel.com> Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.8.3 (2017-05-23) Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <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: Mon, 18 Sep 2017 11:05:02 -0000 On Mon, Sep 18, 2017 at 11:57:03AM +0100, Ananyev, Konstantin wrote: > > > > -----Original Message----- > > From: Richardson, Bruce > > Sent: Monday, September 18, 2017 11:32 AM > > To: Thomas Monjalon <thomas@monjalon.net> > > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; stephen@networkplumber.org; dev@dpdk.org; Shahaf Shuler > > <shahafs@mellanox.com> > > Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API > > > > On Thu, Sep 14, 2017 at 10:02:26AM +0200, Thomas Monjalon wrote: > > > 13/09/2017 23:42, Ananyev, Konstantin: > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > > > 13/09/2017 14:56, Ananyev, Konstantin: > > > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > > > Konstantin, I would like your opinion about the proposal below. > > > > > It is about making on the fly configuration more generic. > > > > > You say it is possible to configure VLAN on the fly, > > > > > and I think we should make it possible for other offload features. > > > > > > > > It would be a good thing, but I don't think it is possible for all offloads. > > > > For some of them you still have to stop the queue(port) first. > > > > > > > > Also I am not sure what exactly do you propose? > > > > Is that something like that: > > > > - wipe existing offload bitfileds from rte_eth_rxmode (already done by Shahaf) > > > > - Instead of uint64_t offloads inside both rte_eth_rxmode and te_eth_rxconf > > > > Introduce new functions: > > > > > > > > int rte_eth_set_port_rx_offload(portid, uint64_t offload_mask); > > > > int rte_eth_set_queue_rx_offload(portid, queueid, uint64_t offload_mask); > > Would be useful to have a valid mask here, to indicate what bits to use. > > That way, you can adjust one bit without worrying about what other bits > > you may change in the process. There are probably apps out there that > > just want to toggle a single bit on, and off, at runtime while ignoring > > others. > > Alternatively, we can have set/unset functions which enable/disable > > offloads, based on the mask. > > My thought was that people would do: > > uint64_t offload = rte_eth_get_port_rx_offload(port); > offload |= RX_OFFLOAD_X; > offload &= ~RX_OFFLOAD_Y; > rte_eth_set_port_rx_offload(port, offload); > > In that case, I think we don't really need a mask. > > > > > > > > > > > uint64_t rte_eth_get_port_rx_offload(portid); > > > > uint64_t rte_eth_set_queue_rx_offload(portid, queueid); > > s/set/get/ > > > > > > > > And add new fileds: > > > > rx_offload_port_dynamic_capa > > > > rx_offload_queue_dynamic_capa > > > > inside rte_eth_dev_info. > > > > > > > > And it would be user responsibility to call set_port/queue_rx_offload() > > > > somewhere before dev_start() for static offloads. > > > > ? > > > > > > Yes exactly. > > > > > > > If so, then it seems reasonable to me. > > > > > > Good, thank you > > > > > > > > Sorry I'm a bit late to the review, but the above suggestion of separate > > APIs for enabling offloads, seems much better than passing in the flags > > in structures to the existing calls. From what I see all later revisions > > of this patchset still use the existing flags parameter to setup calls > > method. > > > > Some advantages that I see of the separate APIs: > > * allows some settings to be set before start, and others afterwards, > > with an appropriate return value if dynamic config not supported. > > * we can get fine grained error reporting from these - the set calls can > > all return the mask indicating what offloads could not be applied - > > zero means all ok, 1 means a problem with that setting. This may be > > easier for the app to use than feature discovery in some cases. > > * for those PMDs which support configuration at a per-queue level, it > > can allow the user to specify the per-port settings as a default, and > > then override that value at the queue level, if you just want one queue > > different from the rest. > > I think we all in favor to have a separate API here. > Though from the discussion we had at latest TB, I am not sure it is doable > in 17.11 timeframe. Ok, so does that imply no change in this release, and that the existing set is to be ignored? /Bruce