On 5/29/2019 9:16 AM, Nithin Dabilpuram wrote: > Hi Thomas, > > On Mon, May 20, 2019 at 06:20:53PM +0530, Nithin Dabilpuram wrote: >> On Fri, May 17, 2019 at 10:59:38AM +0200, Thomas Monjalon wrote: >>> 17/05/2019 10:55, Nithin Dabilpuram: >>>> On Wed, May 15, 2019 at 09:27:22AM +0200, Thomas Monjalon wrote: >>>>> 15/05/2019 08:52, Nithin Dabilpuram: >>>>>> Hi Thomas, >>>>>> On Tue, May 14, 2019 at 05:39:30PM +0200, Thomas Monjalon wrote: >>>>>>> Hi, >>>>>>> >>>>>>> 13/05/2019 13:21, Nithin Dabilpuram: >>>>>>>> With the latest published interface of >>>>>>>> rte_eal_hotplug_[add,remove](), and rte_eth_dev_close(), >>>>>>>> rte_eth_dev_close() would cleanup all the data structures of >>>>>>>> port's eth dev leaving the device common resource intact >>>>>>>> if RTE_ETH_DEV_CLOSE_REMOVE is set in dev flags. >>>>>>>> So "port detach" (~hotplug remove) should be able to work, >>>>>>>> with device identifier like "port attach" as eth_dev could have >>>>>>>> been closed already and rte_eth_devices[port_id] reused. >>>>>>> >>>>>>> "port attach" uses devargs as identifier because there >>>>>>> is no port id before creating it. But "detach port" uses >>>>>>> logically the port id to close. >>>>>> >>>>>> But if "port close" was already called on that port, >>>>>> eth_dev->state would be set as RTE_ETH_DEV_UNUSED and >>>>>> that port id could be reused. >>>>>> So after "port close" if we call "port detach", isn't it >>>>>> incorrect to use the same port id ? >>>>> >>>>> Yes it is incorrect to close a port which is already closed :) >>>>> >>>>>>>> This change alters "port detach" cmdline interface to >>>>>>>> work with device identifier like "port attach". >>>>>>> >>>>>>> The word "port" means an ethdev port, so it should be >>>>>>> referenced with a port id. >>>>>>> If you want to close an EAL rte_device, then you should >>>>>>> rename the command. >>>>>>> But testpmd purpose should be to work with ethdev ports only. >>>>>> >>>>>> Renaming the command to "detach <identifier>" ? >>>>> >>>>> Yes something like that. >>>>> But why do you want to manage rte_device in testpmd? >>>>> Being able to close ports in not enough? >>>>> Please describe a scenario. >>>>> >>>> >>>> We just want to support testing hotplug detach along with >>>> hotplug attach from testpmd. Currently there is no way to detach >>>> if we close the port first. >>> >>> OK >> So can I send next revision with command renamed to "detach <identifier>" ? > > Any info on this ? I can even add it as another cmd without disturbing > existing > command if needed.
This sounds better option to me. I see the need to remove device via 'identifier' but also still it is easier to use 'port_id' for removal when applicable, so I am for keeping it. What do you think adding a new command: 'device detach' Also testpmd doesn't dead with 'device' much, it mainly works in port level, because of this does it make sense to add another command something like: "show device info all" > >>> >>>> Another reason is that in our new PMD, for detaching one specific port, >>>> we need more than one try as the PMD might return -EAGAIN. >>>> So with the current "port detach" implementation, after closing the port, >>>> if PMD returns -EAGAIN for rte_dev_remove() call, there is no way to >>>> try it again. >>> >>> This is a bug. >>> Should we catch -EAGAIN somewhere? >> >> It is already caught in local_dev_remove() and >> rte_dev_remove() fails. Only problem as I said below is >> in testpmd if first call to detach_port_device() i.e handler of "port >> detach", >> rte_dev_remove() returns -EAGAIN and PMD cleaned up the resources partially >> like eth_dev >> resources, the second time call cannot work port_id will not be valid >> anymore. >> >>> >>>
