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

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

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, September 13, 2017 2:21 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; stephen@networkplumber.org
> Cc: 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
> 
> 13/09/2017 14:56, Ananyev, Konstantin:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > 13/09/2017 13:16, Shahaf Shuler:
> > > > Wednesday, September 13, 2017 12:28 PM, Thomas Monjalon:
> > > > > I still think we must streamline ethdev API instead of complexifying.
> > > > > We should drop the big "configure everything" and configure offloads one by
> > > > > one, and per queue (the finer grain).
> > > >
> > > > The issue is, that there is some functionality which cannot be achieved when configuring offload per queue.
> > > > For example - vlan filter on intel NICs. The PF can set it even without creating a single queue, in order to enable it for the VFs.
> > >
> > > As it is a device-specific - not documented - side effect,
> > > I won't consider it.
> >
> > Hmm, are you saying that if there are gaps in our documentation it ok to break things?
> 
> If it is not documented, we did not explicitly agree on it.
> How an application knows that setting a PF settings will have
> effect on related VFs?
> 
> > Once again - you suggest to break existing functionality without providing any
> > alternative way to support it.
> 
> It is not a functionality, it is a side effect.

I wouldn't agree with that.
DPDK does support PF for these devices.
It is responsibility of PF to provide to user ability to configure and control it's VF(s).  

> What happens if a VF changes this settings? error?

Depends on particular offload and HW.
For ixgbe and igb for most cases VF is simply not physically capable to change
these things.
I think, that in some places error is returned, in other such request is silently ignored.

> Is this error documented?
> 
> > Surely I will NACK such proposal.
> 
> Nothing to nack, I agree with v3 which doesn't break ixgbe VLAN settings.

Ok then.

> 
> 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);

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

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.
?

If so, then it seems reasonable to me.
Konstantin

> 
> > > However I understand it may be better to be able to configure
> > > per-port offloads with a dedicated per-port function.
> > > I agree with the approach of the v3 of this series.
> > >
> > > Let me give my overview of offloads:
> > >
> > > We have simple offloads which are configured by just setting a flag.
> > > The same flag can be set per-port or per-queue.
> > > This offload can be set before starting or on the fly.
> > > We currently have no generic way to set it on the fly.
> > >
> > > We have also more complicate offloads which require more configuration.
> > > They are set with the rte_flow API.
> > > They can be per-port, per-queue, on the fly or not (AFAIK).
> > >
> > > I think we must discuss "on the fly" capability.
> > > It requires probably to set up simple offloads (flags) with a dedicated
> > > function instead of using "configure" and "queue_setup" functions.
> > > This new capability can be implemented in a different series.
> > >
> > > Opinions?