app/testpmd: change port detach interface
Checks
Commit Message
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.
This change alters "port detach" cmdline interface to
work with device identifier like "port attach".
Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
---
app/test-pmd/cmdline.c | 15 ++++++++-------
app/test-pmd/testpmd.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
app/test-pmd/testpmd.h | 1 +
3 files changed, 54 insertions(+), 7 deletions(-)
Comments
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.
> 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.
PS: Please remind that a device can have multiple ports.
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 ?
>
> > 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>" ?
>
> PS: Please remind that a device can have multiple ports.
>
>
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.
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.
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.
>
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
> 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?
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>" ?
>
> > 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.
>
>
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.
> >
> > > 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.
>
> >
> >
Hi Thomas,
A reminder about this patch.
On Wed, May 29, 2019 at 01:46:20PM +0530, 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.
>
> > >
> > > > 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.
> >
> > >
> > >
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.
>>
>>>
>>>
On Tue, Jul 02, 2019 at 04:58:17PM +0100, Yigit, Ferruh wrote:
> 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.
Thanks. Will send out a patch for the same.
>
> 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"
Sure. Will add it.
>
> >
> >>>
> >>>> 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.
> >>
> >>>
> >>>
>
@@ -1456,7 +1456,7 @@ cmdline_parse_inst_t cmd_operate_attach_port = {
struct cmd_operate_detach_port_result {
cmdline_fixed_string_t port;
cmdline_fixed_string_t keyword;
- portid_t port_id;
+ cmdline_fixed_string_t identifier;
};
static void cmd_operate_detach_port_parsed(void *parsed_result,
@@ -1466,7 +1466,7 @@ static void cmd_operate_detach_port_parsed(void *parsed_result,
struct cmd_operate_detach_port_result *res = parsed_result;
if (!strcmp(res->keyword, "detach"))
- detach_port_device(res->port_id);
+ detach_port(res->identifier);
else
printf("Unknown parameter\n");
}
@@ -1477,18 +1477,19 @@ cmdline_parse_token_string_t cmd_operate_detach_port_port =
cmdline_parse_token_string_t cmd_operate_detach_port_keyword =
TOKEN_STRING_INITIALIZER(struct cmd_operate_detach_port_result,
keyword, "detach");
-cmdline_parse_token_num_t cmd_operate_detach_port_port_id =
- TOKEN_NUM_INITIALIZER(struct cmd_operate_detach_port_result,
- port_id, UINT16);
+cmdline_parse_token_string_t cmd_operate_detach_port_identifier =
+ TOKEN_STRING_INITIALIZER(struct cmd_operate_detach_port_result,
+ identifier, NULL);
cmdline_parse_inst_t cmd_operate_detach_port = {
.f = cmd_operate_detach_port_parsed,
.data = NULL,
- .help_str = "port detach <port_id>",
+ .help_str = "port detach <identifier>:"
+ "(identifier: pci address or virtual dev name)",
.tokens = {
(void *)&cmd_operate_detach_port_port,
(void *)&cmd_operate_detach_port_keyword,
- (void *)&cmd_operate_detach_port_port_id,
+ (void *)&cmd_operate_detach_port_identifier,
NULL,
},
};
@@ -2453,6 +2453,51 @@ detach_port_device(portid_t port_id)
}
void
+detach_port(char *identifier)
+{
+ struct rte_dev_iterator iterator;
+ struct rte_devargs da;
+ portid_t port_id;
+
+ printf("Removing a device...\n");
+
+ memset(&da, 0, sizeof(da));
+ if (rte_devargs_parsef(&da, "%s", identifier)) {
+ printf("cannot parse identifier\n");
+ if (da.args)
+ free(da.args);
+ return;
+ }
+
+ RTE_ETH_FOREACH_MATCHING_DEV(port_id, identifier, &iterator) {
+ if (ports[port_id].port_status != RTE_PORT_CLOSED) {
+ if (ports[port_id].port_status != RTE_PORT_STOPPED) {
+ printf("Port %u not stopped\n", port_id);
+ return;
+ }
+
+ /* sibling ports are forced to be closed */
+ if (ports[port_id].flow_list)
+ port_flow_flush(port_id);
+ ports[port_id].port_status = RTE_PORT_CLOSED;
+ printf("Port %u is now closed\n", port_id);
+ }
+ }
+
+ if (rte_eal_hotplug_remove(da.bus->name, da.name) != 0) {
+ TESTPMD_LOG(ERR, "Failed to detach device %s(%s)\n",
+ da.name, da.bus->name);
+ return;
+ }
+
+ remove_invalid_ports();
+
+ printf("Device %s is detached\n", identifier);
+ printf("Now total ports is %d\n", nb_ports);
+ printf("Done\n");
+}
+
+void
pmd_test_exit(void)
{
struct rte_device *device;
@@ -788,6 +788,7 @@ void stop_port(portid_t pid);
void close_port(portid_t pid);
void reset_port(portid_t pid);
void attach_port(char *identifier);
+void detach_port(char *identifier);
void detach_port_device(portid_t port_id);
int all_ports_stopped(void);
int port_is_stopped(portid_t port_id);