Hi Ferruh From: Ferruh Yigit, Wednesday, April 25, 2018 3:54 PM > On 4/25/2018 1:16 PM, Matan Azrad wrote: > > Hi all > > > > From: Ferruh Yigit, Thursday, April 19, 2018 2:08 PM > >>> But rte_eth_dev_release_port() is still broken because of this > >>> change, please check _rte_eth_dev_callback_process() which uses > >>> dev->data- port_id. > > > > The issue is that a DESTROY callback gets port_id=0 all the time, regardless > the destroyed port id. > > > > Let's discuss about the fix: > > > > There are 2 options for the DESTROY event meaning: > > > > 1. The device is going to be destroyed in the future (a bit after the > > callbacks > calling). > > The user may think that there is a valid data in the device structure in > the callback time, > > Thus, he may use it. > > The fix here is to move the callback to the start of the function, > > In this time the data field is still valid. > > > > 2. The device was already destroyed in the past (a bit before the callbacks > calling). > > The user should think that there is no any valid data in the device > structure in the callback time, > > Thus, he doesn't use it. > > The issue here: > > _rte_eth_dev_callback_process() assumes there is a valid data in the > data field all the time, > > But in this case the data field is not valid because the device was > already destroyed. > > Optional fixes: > > 1. Always keep the data->port_id valid. > > 2. keep the data->port_id valid only for the > _rte_eth_dev_callback_process() call. > > 3. Change _rte_eth_dev_callback_process() arg from "struct > rte_eth_dev *dev" to "uint16_t port_id" > > a. Need to change all the calls for this internal API. > > > > I vote to 2.1. > > > > > > What do you think? > > What is the concern with 1? It is easy to implement. > Yes, also 2.1 and 2.2 are easy.
> And it may be better because if callback called after device destroyed, there > is no guarantee/locking that same port won't be re-used, in the middle of the > callback function rte_eth_dev_data can be updated, no? > Good point! I think we must guarantee no port allocation for the same port id in the callback time. I also prefer to not call the callbacks in the critical section. So maybe call it before the locking is better.
