Re: [PATCH 4/4] ethdev: add helpers to move to the new offloads API

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Monday, September 18, 2017 12:05 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; 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 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?

No, my understanding the current plan is to go forward with Shahaf patches,
and then apply another one (new set/get API) on top of them.
Konstantin