Re: [dpdk-dev] [PATCH v2 6/6] app/testpmd: adjust ethdev port ownership

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

On Mon, Jan 08, 2018 at 01:55:52PM +0000, Matan Azrad wrote:
> Hi Gaetan
> 
> From: Gaëtan Rivet, Monday, January 8, 2018 3:30 PM
> > On Mon, Jan 08, 2018 at 12:30:19PM +0000, Matan Azrad wrote:
> > >
> > >
> > > From: Gaëtan Rivet, Monday, January 8, 2018 1:40 PM
> > > > Hi Matan,
> > > >
> > > > On Sun, Jan 07, 2018 at 09:45:51AM +0000, Matan Azrad wrote:
> > > > > Testpmd should not use ethdev ports which are managed by other
> > > > > DPDK entities.
> > > > >
> > > > > Set Testpmd ownership to each port which is not used by other
> > > > > entity and prevent any usage of ethdev ports which are not owned by
> > Testpmd.
> > > > >
> > > >
> > > > This patch should not be necessary.
> > > >
> > > > Ideally, your API evolution should not impact the default case. As
> > > > such, the default iterator RTE_ETH_FOREACH_DEV should still be used in
> > testpmd.
> > > >
> > > Why? We want to adjust testpmd to the port ownership.
> > >
> > 
> > This adjustment should be seamless for existing application.
> > 
> > > > RTE_ETH_FOREACH_DEV should call
> > RTE_ETH_FOREACH_DEV_OWNED_BY, with
> > > > the default owner (meaning that it would thus iterate on the
> > > > application-owned set of device).
> > > >
> > >
> > > It will break the API (we already talked about it).
> > > There is not any default owner:
> > > Any DPDK entity includes applications must to allocate an owner ID and use
> > it to own the ports they wants to use.
> > > The application can include more than 1 owner depends on the user needs.
> > > Each DPDK entity which can synchronize all its port usage can be a valid
> > DPDK entity for the ownership mechanism.
> > >
> > 
> > That's the point of my remark: you did not include a default owner.
> > I think there should be one, and that all ports should pertain to this default
> > owner by default when created.
> > 
> > This would not prevent a user or application from adding new owners specific
> > to their use and specialize ports if need be.
> > 
> > However, for other applications that do not care for this specialization, they
> > should run with the current API and avoid the ports that are configured by
> > other third parties.
> > 
> 
> RTE_ETH_FOREACH_DEV means iterate over all devices and should stay as is in my opinion.
> I understand your concern about changes in current application,
> But your "default" suggestion will cause to "non-default" applications to reset all the default owners and will complicate them and hurt semantics.

Why should an application be able to iterate over all ports? Leave this
capability to the EAL (or ethdev layer) alone, while other components should
be restricted to their specific set.

And if a need for this general iterator appears, solutions could be
found very easily.

RTE_ETH_FOREACH_DEV currently does not iterate over deferred ports, it
iterates over the base set of ports available. Changing this behavior is
not necessary, you could introduce your API while keeping it.

> 
> > I'm thinking about applications already written that would be used with fail-
> > safe ports: they would use RTE_ETH_FOREACH_DEV, and would thus iterate
> > over every ports, including those owned by the fail-safe, unless they start
> > following the new API.
> > 
> 
> They should start, it is really not complicated.

The point is not whether developpers downstream would be able to grasp
such complexity, but whether a project like DPDK should foster an
unstable environment for its currently still limited ecosystem.

> What's about application which use count=rte_eth_dev_count and iterate over all ports from 0 to count-1?
> We cannot save all the wrong application options.
> 
> > This is unnecessary: adding a default owner for all created ports and
> > redefining RTE_ETH_FOREACH_DEV as follow
> > 
> > #define RTE_ETH_FOREACH_DEV(i)
> >         RTE_ETH_FOREACH_DEV_OWNED_BY(i, RTE_ETH_DEFAULT_OWNER)
> > 
> > Is simple enough and will simplify the work of DPDK users. Moreover, it
> > would make fail-safe compatible with all applications using
> > RTE_ETH_FOREACH_DEV without additional evolution. It would actually make
> > any code using your API supported by those same applications, which I think
> > would help its adoption.
> > 
> 
> Will break API, will hurt semantic of FOREACH , and will complicate ownership care applications as I wrote above.

Well, breaking an API is best before such API is integrated anyway.

I disagree regarding the added complexity for applications that would
use ownership. With your proposal, most applications will only add a
single user and register all their ports with this user, then be forced
to iterate upon their registered user.

You can save all of them the hassle of adding this code, by taking care
of the most common case, avoiding redundant code downstream and simplifying
possible future update to this default case.

So if anything, this would greatly simplify ownership for the vast
majority of applications.

-- 
Gaëtan Rivet
6WIND