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

[Matan Azrad <matan@mellanox.com>]

Hi Konstantine

> Hi Matan,
> 
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > 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.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Under race condition - in the worst case it
> > > > > > > > > > > > > > might crash, though for that you'll have to be really
> unlucky.
> > > > > > > > > > > > > > Though in most cases as you said it would just
> > > > > > > > > > > > > > not operate
> > > > > > > > correctly.
> > > > > > > > > > > > > > I think if we start to protect dev->name by
> > > > > > > > > > > > > > lock we need to do it for all instances (both read and
> write).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > Since under the ownership rules, the user must
> > > > > > > > > > > > > take ownership
> > > > > > of a
> > > > > > > > > > > > > port
> > > > > > > > > > > > before using it, I still don't see a problem here.
> > > > > > > > > > > >
> > > > > > > > > > > > I am not talking about owner id or name here.
> > > > > > > > > > > > I am talking about dev->name.
> > > > > > > > > > > >
> > > > > > > > > > > So? The user still should take ownership of a device
> > > > > > > > > > > before using it
> > > > > > (by
> > > > > > > > > > name or by port id).
> > > > > > > > > > > It can just read it without owning it, but no managing it.
> > > > > > > > > > >
> > > > > > > > > > > > > Please, Can you describe specific crash scenario
> > > > > > > > > > > > > and explain how could the
> > > > > > > > > > > > locking fix it?
> > > > > > > > > > > >
> > > > > > > > > > > > Let say thread 0 doing rte_eth_dev_allocate()-
> > > > > > > > > > > > >snprintf(rte_eth_dev_data[x].name, ...), thread 1
> > > > > > > > > > > > >doing
> > > > > > > > > > > > rte_pmd_ring_remove()->rte_eth_dev_allocated()-
> > > > >strcmp().
> > > > > > > > > > > > And because of race condition -
> > > > > > > > > > > > rte_eth_dev_allocated() will
> > > > > > return
> > > > > > > > > > > > rte_eth_dev * for the wrong device.
> > > > > > > > > > > Which wrong device do you mean? I guess it is the
> > > > > > > > > > > device which
> > > > > > > > currently is
> > > > > > > > > > being created by thread 0.
> > > > > > > > > > > > Then rte_pmd_ring_remove() will call rte_free()
> > > > > > > > > > > > for related resources, while It can still be in
> > > > > > > > > > > > use by someone
> > > else.
> > > > > > > > > > > The rte_pmd_ring_remove caller(some DPDK entity)
> > > > > > > > > > > must take
> > > > > > > > ownership
> > > > > > > > > > > (or validate that he is the owner) of a port before
> > > > > > > > > > > doing it(free,
> > > > > > > > release), so
> > > > > > > > > > no issue here.
> > > > > > > > > >
> > > > > > > > > > Forget about ownership for a second.
> > > > > > > > > > Suppose we have a process it created ring port for
> > > > > > > > > > itself (without
> > > > > > setting
> > > > > > > > any
> > > > > > > > > > ownership)  and used it for some time.
> > > > > > > > > > Then it decided to remove it, so it calls
> > > > > > > > > > rte_pmd_ring_remove()
> > > > for it.
> > > > > > > > > > At the same time second process decides to call
> > > > > > rte_eth_dev_allocate()
> > > > > > > > (let
> > > > > > > > > > say for anither ring port).
> > > > > > > > > > They could collide trying to read (process 0) and
> > > > > > > > > > modify (process 1)
> > > > > > same
> > > > > > > > > > string rte_eth_dev_data[].name.
> > > > > > > > > >
> > > > > > > > > Do you mean that process 0 will compare successfully the
> > > > > > > > > process 1
> > > > > > new
> > > > > > > > port name?
> > > > > > > >
> > > > > > > > Yes.
> > > > > > > >
> > > > > > > > > The state are in local process memory - so process 0
> > > > > > > > > will not compare
> > > > > > the
> > > > > > > > process 1 port, from its point of view this port is in
> > > > > > > > UNUSED
> > > > > > > > > state.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Ok, and why it can't be in attached state in process 0 too?
> > > > > > >
> > > > > > > Someone in process 0 should attach it using protected
> > > > > > > attach_secondary
> > > > > > somewhere in your scenario.
> > > > > >
> > > > > > Yes, process 0 can have this port attached too, why not?
> > > > > See the function with inline comments:
> > > > >
> > > > > struct rte_eth_dev *
> > > > > rte_eth_dev_allocated(const char *name) {
> > > > > 	unsigned i;
> > > > >
> > > > > 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> > > > >
> > > > > 	    	The below state are in local process memory,
> > > > > 		So, if here process 1 will allocate a new port (the current
> > > > > i),
> > > > update its local state to ATTACHED and write the name,
> > > > > 		the state is not visible by process 0 until someone in process
> > > > 0 will attach it by rte_eth_dev_attach_secondary.
> > > > > 		So, to use rte_eth_dev_attach_secondary process 0 must
> > > > take the lock
> > > > > and it can't, because it is currently locked by process 1.
> > > >
> > > > Ok I see.
> > > > Thanks for your patience.
> > > > BTW, that means that if let say process 0 will call
> > > > rte_eth_dev_allocate("xxx") and process 1 will call
> > > > rte_eth_dev_allocate("yyy") we can endup with same port_id be used
> > > > for different devices and 2 processes will overwrite the same
> > > rte_eth_dev_data[port_id]?
> > >
> > > No, contrary to the state, the lock itself is in shared memory, so 2
> > > processes cannot allocate port in the same time.(you can see it in
> > > the next patch of this series).
> 
> I am not talking about racing here.
> Let say process 0 calls rte_pmd_ring_probe()->....-
> >rte_eth_dev_allocate("xxx")
> rte_eth_dev_allocate() finds that port N is 'free', i.e.
> local rte_eth_devices[N].state == RTE_ETH_DEV_UNUSED so it assigns new
> dev ("xxx") to port N.
> Then after some time process 1 calls rte_pmd_ring_probe()->....-
> >rte_eth_dev_allocate("yyy").
> From its perspective port N is still free:  rte_eth_devices[N].state ==
> RTE_ETH_DEV_UNUSED, so it will assign new dev ("yyy") to the same port.
> 

Yes you right, this is a problem(not related actually to port ownership) but look:
As I understand the secondary processes are not allowed to create a ports and they must to use attach_secondary API, but there is not hardcoded check which prevent them to do it.


Konstantin
> 
> 
> > >
> >
> > Actually I think only one process(primary) should allocate ports, the others
> should attach them.
> > The race of port allocation is only between the threads of the primary
> process.
> >
> >
> > > > Konstantin
> > > >
> > > > >
> > > > > 		if ((rte_eth_devices[i].state == RTE_ETH_DEV_ATTACHED)
> > > > &&
> > > > > 		strcmp(rte_eth_devices[i].data->name, name) == 0)
> > > > > 			return &rte_eth_devices[i];
> > > > > 	}
> > > > > 	return NULL;
> > > > >
> > > > >