[dpdk-dev] [PATCH v3] net/mlx4: fix dev rmv not detected after port stop

[Matan Azrad]

Hi all

From: Adrien Mazarguil
> On Tue, Jan 30, 2018 at 08:37:06PM +0000, Shahaf Shuler wrote:
> > Tuesday, January 30, 2018 11:40 AM, Adrien Mazarguil:
> > > Unfortunately I didn't get a chance to review this patch before it was
> applied.
> > > I'm not sure a stopped port is supposed to report events
> > > (interrupts). Will applications expect them to occur at this point?
> >
> > Why not?
> >
> > Stopped port is still counted as attached. The fact the application stopped
> the packet receive on it doesn't mean it should not receive a sync events
> (such as the remove event).
> > async events, by definition, are not related to traffic being flows through
> the port.
> 
> My comment is based on my understanding of rte_eth_dev_stop(), which is
> a device (or port) is completely stopped, in a suspended state and no
> interrupts shall occur, as a means for applications to temporarily not be
> bothered by them until restarted.

Stopping traffic is not saying that the application is not interesting in the device,
I think that you mean to dev_close().

Any event may still be usable for application between dev_stop() to dev_start(), 
especially RMV or LCS can still be interested.

> Think about it that way: applications do not want to get interrupts
> immediately after the device is initialized, because they might not be ready
> to process them at this point. An explicit call to rte_eth_dev_start() tells the
> PMD when it's OK to do so. The converse is rte_eth_dev_stop().

So, they can delay the event registration to the time they interesting in the events.
And use event unregister when they are not interesting in it anymore.

> Stopping traffic can already be achieved by not polling from the application
> side, calling rte_eth_dev_[rt]x_queue_stop() and/or toggling RX/TX
> interrupts through rte_eth_dev_[rt]x_intr_enable(). rte_eth_dev_stop()
> provides lower-level device control.
 
I think it makes sense only for Rx interrupt which is traffic oriented(like stop and start). 

> Perhaps documentation is not clear, however that's how LSC seems
> implemented in all PMDs; it gets disabled after rte_eth_dev_stop() and one
> should explicitly use rte_eth_link_get() to retrieve link status afterward. I
> think RMV should behave similarly with rte_eth_dev_is_removed().
> Adapting fail-safe should be easier than modifying all the remaining PMDs.

Or maybe PMDs which do it make mistakes.

Matan.

> > > In my opinion it's not a fix, as in, it doesn't address an issue
> > > introduced by the mentioned patch whose behavior was correct.
> > >
> > > It's probably too late to change it now and it does address an issue
> > > seen with a use case involving this PMD, however I think the
> > > fail-safe PMD could as well poll using the recently-added
> > > rte_eth_dev_is_removed() when it's aware the underlying port is
> stopped instead of expecting interrupts.
> > >
> > > --
> > > Adrien Mazarguil
> > > 6WIND
> 
> --
> Adrien Mazarguil
> 6WIND