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

[Bruce Richardson <bruce.richardson@intel.com>]

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