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 F3CE7325A for ; Mon, 18 Sep 2017 12:31:59 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Sep 2017 03:31:59 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,412,1500966000"; d="scan'208";a="1173251060" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.24]) by orsmga001.jf.intel.com with SMTP; 18 Sep 2017 03:31:56 -0700 Received: by (sSMTP sendmail emulation); Mon, 18 Sep 2017 11:31:55 +0100 Date: Mon, 18 Sep 2017 11:31:55 +0100 From: Bruce Richardson To: Thomas Monjalon Cc: "Ananyev, Konstantin" , stephen@networkplumber.org, dev@dpdk.org, Shahaf Shuler Message-ID: <20170918103154.GA13172@bricha3-MOBL3.ger.corp.intel.com> References: <12544923.ZPp1eIik3W@xps> <2601191342CEEE43887BDE71AB9772584F24AA9D@irsmsx105.ger.corp.intel.com> <6120098.SpXJOblVKo@xps> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6120098.SpXJOblVKo@xps> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Sep 2017 10:32:00 -0000 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. > > > > 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. Regards, /Bruce