[dpdk-dev] [PATCH v2 2/6] ethdev: add port ownership

[Matan Azrad]

Hi Konstantin

From: Ananyev, Konstantin, Friday, January 12, 2018 2:02 AM
> Hi Matan,
> 
> >
> > Hi Konstantin
> >
> > From: Ananyev, Konstantin, Thursday, January 11, 2018 2:40 PM
> > > Hi Matan,
> > >
> > > >
> > > > Hi Konstantin
> > > >
> > > > From: Ananyev, Konstantin, Wednesday, January 10, 2018 3:36 PM
> > > > > Hi Matan,
<snip>
> > > > > Few comments from me below.
> > > > > BTW, do you plan to add ownership mandatory check in control
> > > > > path functions that change port configuration?
> > > >
> > > > No.
> > >
> > > So it still totally voluntary usage and application nneds to be
> > > changed to exploit it?
> > > Apart from RTE_FOR_EACH_DEV() change proposed by Gaetan?
> > >
> >
> > Also RTE_FOR_EACH_DEV() change proposed by Gaetan is not protected
> because 2 DPDK entities can get the same port while using it.
> 
> I am not talking about racing condition here.
> Right now even from the same thread - I can call dev_configure() for the port
> which I don't own (let say it belongs to failsafe port), and that would remain,
> correct?
> 
Yes.

> > As I wrote in the log\docs and as discussed a lot in the first version:
> > The new synchronization rules are:
> > 1. The port allocation and port release synchronization will be
> >    managed by ethdev.
> > 2. The port usage synchronization will be managed by the port owner.
> > 3. The port ownership API synchronization(also with port creation) will be
> managed by ethdev.
> > 4. DPDK entity which want to use a port must take ownership before.
> >
> > Ethdev should not protect 2 and 4 according these rules.
> >
> > > > > Konstantin
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Matan Azrad [mailto:matan at mellanox.com]
<snip>
> > I mean the documentation about the needed alignment for spinlock.
> Where is it?
> 
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfo
> center.arm.com%2Fhelp%2Findex.jsp%3Ftopic%3D%2Fcom.arm.doc.faqs%2
> Fka15414.html&data=02%7C01%7Cmatan%40mellanox.com%7Cb3c329ae9db
> f4bd29a7008d5594fb776%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1
> %7C636513121294703050&sdata=40v3b4wk5f4qEyIY5jdDv8S47LjgXK0t9TPtav
> XIMOk%3D&reserved=0
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Finfo
> center.arm.com%2Fhelp%2Findex.jsp%3Ftopic%3D%2Fcom.arm.doc.dht000
> 8a%2FCJAGCFAF.html&data=02%7C01%7Cmatan%40mellanox.com%7Cb3c32
> 9ae9dbf4bd29a7008d5594fb776%7Ca652971c7d2e4d9ba6a4d149256f461b%7
> C0%7C1%7C636513121294703050&sdata=B7pEZjFJntVp3Il8fS9wr%2FlxABgNX
> FSr9PE4emEPLQE%3D&reserved=0
> 
> Might be ARM and PPC guys can provide you some more complete/recent
> docs.
Thanks.
<snip> 
> > > > > It is good to see that now scanning/updating rte_eth_dev_data[]
> > > > > is lock protected, but it might be not very plausible to protect
> > > > > both data[] and next_owner_id using the same lock.
> > > >
> > > > I guess you mean to the owner structure in rte_eth_dev_data[port_id].
> > > > The next_owner_id is read by ownership APIs(for owner validation),
> > > > so it
> > > makes sense to use the same lock.
> > > > Actually, why not?
> > >
> > > Well to me next_owner_id and rte_eth_dev_data[] are not directly
> related.
> > > You may create new owner_id but it doesn't mean you would update
> > > rte_eth_dev_data[] immediately.
> > > And visa-versa - you might just want to update
> > > rte_eth_dev_data[].name or .owner_id.
> > > It is not very good coding practice to use same lock for non-related
> > > data structures.
> > >
> > I see the relation like next:
> > Since the ownership mechanism synchronization is in ethdev
> > responsibility, we must protect against user mistakes as much as we can by
> using the same lock.
> > So, if user try to set by invalid owner (exactly the ID which currently is
> allocated) we can protect on it.
> 
> Hmm, not sure why you can't do same checking with different lock or atomic
> variable?
> 
The set ownership API is protected by ownership lock and checks the owner ID validity 
By reading the next owner ID.
So, the owner ID allocation and set API should use the same atomic mechanism.
The set(and others) ownership APIs already uses the ownership lock so I think it makes sense to use the same lock also in ID allocation.
 
> > > > > In fact, for next_owner_id, you don't need a lock - just
> > > > > rte_atomic_t should be enough.
> > > >
> > > > I don't think so, it is problematic in next_owner_id wraparound
> > > > and may
> > > complicate the code in other places which read it.
> > >
> > > IMO it is not that complicated, something like that should work I think.
> > >
> > > /* init to 0 at startup*/
> > > rte_atomic32_t *owner_id;
> > >
> > > int new_owner_id(void)
> > > {
> > >     int32_t x;
> > >     x = rte_atomic32_add_return(&owner_id, 1);
> > >     if (x > UINT16_MAX) {
> > >        rte_atomic32_dec(&owner_id);
> > >        return -EOVERWLOW;
> > >     } else
> > >         return x;
> > > }
> > >
> > >
> > > > Why not just to keep it simple and using the same lock?
> > >
> > > Lock is also fine, I just think it better be a separate one - that
> > > would protext just next_owner_id.
> > > Though if you are going to use uuid here - all that probably not
> > > relevant any more.
> > >
> >
> > I agree about the uuid but still think the same lock should be used for both.
> 
> But with uuid you don't need next_owner_id at all, right?
> So lock will only be used for rte_eth_dev_data[] fields anyway.
>
Sorry, I meant uint64_t, not uuid.

> > > > > Another alternative would be to use 2 locks - one for
> > > > > next_owner_id second for actual data[] protection.
> > > > >
> > > > > Another thing - you'll probably need to grab/release a lock
> > > > > inside
> > > > > rte_eth_dev_allocated() too.
> > > > > It is a public function used by drivers, so need to be protected too.
> > > > >
> > > >
> > > > Yes, I thought about it, but decided not to use lock in next:
> > > > rte_eth_dev_allocated
> > > > rte_eth_dev_count
> > > > rte_eth_dev_get_name_by_port
> > > > rte_eth_dev_get_port_by_name
> > > > maybe more...
> > >
> > > As I can see in patch #3 you protect by lock access to
> > > rte_eth_dev_data[].name (which seems like a good  thing).
> > > So I think any other public function that access
> > > rte_eth_dev_data[].name should be protected by the same lock.
> > >
> >
> > I don't think so, I can understand to use the ownership lock here(as in port
> creation) but I don't think it is necessary too.
> > What are we exactly protecting here?
> > Don't you think it is just timing?(ask in the next moment and you  may
> > get another answer) I don't see optional crash.
> 
> Not sure what you mean here by timing...
> As I understand rte_eth_dev_data[].name unique identifies device and is
> used by  port allocation/release/find functions.
> As you stated above:
> "1. The port allocation and port release synchronization will be  managed by
> ethdev."
> To me it means that ethdev layer has to make sure that all accesses to
> rte_eth_dev_data[].name are atomic.
> Otherwise what would prevent the situation when one process does
> rte_eth_dev_allocate()->snprintf(rte_eth_dev_data[x].name, ...) while
> second one does rte_eth_dev_allocated(rte_eth_dev_data[x].name, ...) ?
> 
The second will get True or False and that is it.
Maybe if it had been called just a moment after, It might get different answer. 
Because these APIs don't change ethdev structure(just read), it can be OK.
But again, I can understand to use ownership lock also here.

<snip>
> > > Static allocation is fine by me - I just said there is probably no
> > > need to fail if description provide by use will be truncated in that case.
> > > Though if used description is *that* important - rte_malloc() can help
> here.
> > >
> > Again, what is the difference between port name and owner name
> regarding the allocations?
> 
> As I understand rte_eth_dev_data[].name unique identifies device and
> always has to be consistent.
> owner.name is not critical for system operation, and I don't see a big deal if it
> would be truncated.
> 
> > The advantage of static allocation:
> > 1. Not use protected malloc\free functions in other protected code.
> 
> You can call malloc/free before/after grabbing the lock.
> But as I said - I am fine with static array here too - I just don't think truncating
> user description should cause a failure.
> 

Ok, will just add warning print in truncation case.

> > 2.  Easier to the user.
> >
> > > >
> > > > > > +		memset(port_owner->name, 0,
> > > > > RTE_ETH_MAX_OWNER_NAME_LEN);
> > > > > > +		RTE_LOG(ERR, EAL, "Invalid owner name.\n");
> > > > > > +		ret = -EINVAL;
> > > > > > +		goto unlock;
> > > > > > +	}
> > > > > > +
> > > > > > +	port_owner->id = owner->id;
> > > > > > +	RTE_PMD_DEBUG_TRACE("Port %d owner is %s_%05d.\n",
> port_id,
> > > > > > +			    owner->name, owner->id);
> > > > > > +
> > > > >
> > > > > As another nit - you can avoid all these gotos by restructuring code a
> bit:
> > > > >
> > > > > rte_eth_dev_owner_set(const uint16_t port_id, const struct
> > > > > rte_eth_dev_owner *owner) {
> > > > >     rte_spinlock_lock(...);
> > > > >     ret = _eth_dev_owner_set_unlocked(port_id, owner);
> > > > >     rte_spinlock_unlock(...);
> > > > >     return ret;
> > > > > }
> > > > >
> > > > Don't you like gotos? :)
> > >
> > > Not really :)
> > >
> > > > I personally use it only in error\performance scenarios.
> > >
> > > Same here - prefer to avoid them if possible.
> > >
> > > > Do you think it worth the effort?
> > >
> > > IMO - yes, well structured code is much easier to understand and
> maintain.
> > I don't think so in error cases(and performance), It is really clear here, but if
> you are insisting, I will change it.
> > Are you?
> 
> Yes, that would be my preference.
> Why otherwise I would bother to write all this? :)
> 
> > (If the community thinks like you I think "goto" check should be added to
> checkpatch).
> 
> Might be there are pieces of code there goto are really hard to avoid, and/or
> using goto would provide some performance benefit or so...
> But that case definitely doesn't look like that.

Let's stop "goto" discussion here, in spite of I don't think like you globally, In this case I have no problem to change it. 

Thanks,
Matan.