-----Original Message-----
From: Richardson, Bruce
Sent: Monday, September 18, 2017 12:05 PM
To: Ananyev, Konstantin <redacted>
Cc: Thomas Monjalon <redacted>; stephen@networkplumber.org; dev@dpdk.org; Shahaf Shuler <redacted>
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:

quoted

quoted

-----Original Message-----
From: Richardson, Bruce
Sent: Monday, September 18, 2017 11:32 AM
To: Thomas Monjalon <redacted>
Cc: Ananyev, Konstantin <redacted>; stephen@networkplumber.org; dev@dpdk.org; Shahaf Shuler
[off-list ref]
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:

quoted

13/09/2017 23:42, Ananyev, Konstantin:

quoted

From: Thomas Monjalon [mailto:thomas@monjalon.net]

quoted

13/09/2017 14:56, Ananyev, Konstantin:

quoted

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.

quoted

quoted

quoted

uint64_t rte_eth_get_port_rx_offload(portid);
uint64_t rte_eth_set_queue_rx_offload(portid, queueid);

s/set/get/

quoted

quoted

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.

quoted

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