Hi Konstantin

From: Ananyev, Konstantin, Friday, January 12, 2018 2:02 AM

Hi Matan,

quoted

Hi Konstantin

From: Ananyev, Konstantin, Thursday, January 11, 2018 2:40 PM

quoted

Hi Matan,

quoted

Hi Konstantin

From: Ananyev, Konstantin, Wednesday, January 10, 2018 3:36 PM

quoted

Hi Matan,

<snip>

quoted

quoted

quoted

quoted

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.

quoted

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.

quoted

4. DPDK entity which want to use a port must take ownership before.

Ethdev should not protect 2 and 4 according these rules.

quoted

quoted

quoted

Konstantin

quoted

-----Original Message-----
From: Matan Azrad [mailto:matan@mellanox.com]

<snip>

quoted

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> 

quoted

quoted

quoted

quoted

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.

quoted

Actually, why not?

Well to me next_owner_id and rte_eth_dev_data[] are not directly

related.

quoted

quoted

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.

quoted

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.
 

quoted

quoted

quoted

quoted

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

quoted

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.

quoted

quoted

quoted

quoted

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.

quoted

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>

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

2.  Easier to the user.

quoted

quoted

quoted

quoted

+		memset(port_owner->name, 0,

RTE_ETH_MAX_OWNER_NAME_LEN);

quoted

+		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,

quoted

quoted

quoted

quoted

quoted

+			    owner->name, owner->id);
+

As another nit - you can avoid all these gotos by restructuring code a

bit:

quoted

quoted

quoted

quoted

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

quoted

I personally use it only in error\performance scenarios.

Same here - prefer to avoid them if possible.

quoted

Do you think it worth the effort?

IMO - yes, well structured code is much easier to understand and

maintain.

quoted

I don't think so in error cases(and performance), It is really clear here, but if

you are insisting, I will change it.

quoted

Are you?

Yes, that would be my preference.
Why otherwise I would bother to write all this? :)

quoted

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