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

[Thomas Monjalon <thomas@monjalon.net>]

18/09/2017 20:18, Shahaf Shuler:
> Monday, September 18, 2017 5:45 PM, Bruce Richardson:
> > 
> > On Mon, Sep 18, 2017 at 02:27:25PM +0000, Shahaf Shuler wrote:
> > > Monday, September 18, 2017 2:38 PM, Bruce Richardson
> > > > On Mon, Sep 18, 2017 at 01:32:29PM +0200, Thomas Monjalon wrote:
> > > > > 18/09/2017 13:11, Ananyev, Konstantin:
> > > > > > From: Richardson, Bruce
> > > > > > > >
> > > > > > > > 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.
> > > > >
> > > > > Yes, it is what we agreed (hope to see it in minutes).
> > > > > If someone can do these new patches in 17.11 timeframe, it's great!
> > > > > Bruce, do you want to make it a try?
> > > >
> > > > If I have the chance, I can try, but given how short time is and
> > > > that userspace is on next week, I very much doubt I'll even get it started.
> > >
> > > I wasn't aware to the techboard decision on the extra patchset needed.
> > > I think it will be wrong to introduce an API on 17.11 and change it again on
> > 18.02.
> > > I will do my best to make everything ready for 17.11 so we can have one
> > solid API on top of which all PMDs and application will be converted.
> > Considering some Holidays and the DPDK summit I won't have much time to
> > work on it.
> > >
> > > The plan is as follows:
> > > 1.  complete the last comment on the current series and integrate it.
> > > 2. send a new patchset to convert to the API suggested above.
> > >
> > > Aggregating the different suggestions I come up with the below. if this is
> > agreed, then I will move with the implementation.
> > > (I thought it is good to return error values for the get function).
> > 
> > I'd rather you didn't. :-) The only realistic error I would consider is an invalid
> > port id, and I think returning 0 - no offloads - is fine in those cases. The user
> > will pretty quickly discover it's an invalid port id anyway, so I prefer a get
> > function to just return the value as a return value and be done with it!
> 
> It would be simpler, however am not sure invalid port is the only error to consider. Another possible error can be the PMD is not supporting this function.
> On that case returning 0 is not good enough. The application cannot know why the offload is not set, is it because it is set wrong or the PMD just don't support this API (which leads me to my next point). 

We can skip error reporting on "get" functions
and rely on "set" functions to return error if offload API is not supported
or for other miscellaneous errors.

> Declare:
> API1 = The API being worked so far.
> AP2 = The suggested API being discussed here.
> 
> API1  was designed for easy adoption from both PMDs and application. Application can use either old/new API on top of PMD which support one of them thanks to the convert function. There was no hard demand to force all of the PMDs to support it at once. 
> With API2 this model breaks. Application which moved to the new offloads API cannot work with PMD which supports the old one.

It means the generic applications cannot migrate to the new API
until every PMDs have migrated.
I don't see it like a big issue.

> If I aggregate the pros for API2:
> From Bruce:
> >* 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 that functionality I think the get function are enough (application set offload and then check which of them is actually set).
> The set function are more for the on the fly configuration. 
> 
> >* 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.
> 
> This can be done with API1 as well.  
> 
> From Thomas:
> > make the on the flight vlan offloads configuration more generic.
> 
> The cons:
> 1. hard requirement from the PMDs to support the new API. 
> 
> I can commit for mlx5 and mlx4 PMDs. I know other PMD maintainers plan to convert their PMDs as well. If we take this approach we must make sure they all move.

We can try to get an agreement from more vendors at Dublin summit.
If not, we can wait more than one release cycle for late support.

> There is another possible API (API3):
> 1. keep the per-port, per-queue configuration.
> 2. add the get function for better error reporting and visibility for application.
> 3. keep the current on the flight vlan offloading configuration as an exception. In case there will be a need to configure more offloads on the flight we can move to API2.
> 
> With API1 I am obviously OK.
> I agree API2 is more generic. 
> API3 is a nice compromise, if we don't want to force all PMD to convert. 

The question is: do we want to choose a compromise while breaking this API?