On Wed, Sep 05, 2018 at 04:50:33PM +0200, Ursula Braun wrote:
>
>
> On 08/07/2018 12:19 PM, Leon Romanovsky wrote:
> > On Tue, Aug 07, 2018 at 10:53:13AM +0200, Ursula Braun wrote:
> >> The mlx4 driver does not trigger an IB_EVENT_PORT_ACTIVE when the
> >> RoCE network interface is activated. When SMC determines the RoCE
> >> device port to be used, it checks the port states.
> >> This patch triggers IB events for NETDEV_UP and NETDEV_DOWN.
> >>
> >> Signed-off-by: Ursula Braun <ubraun@xxxxxxxxxxxxx>
> >> ---
> >> drivers/infiniband/hw/mlx4/main.c | 20 ++++++++++++++++++++
> >> 1 file changed, 20 insertions(+)
> >>
> >> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> >> index 4ec519afc45b..81a1fd9e8615 100644
> >> --- a/drivers/infiniband/hw/mlx4/main.c
> >> +++ b/drivers/infiniband/hw/mlx4/main.c
> >> @@ -2447,6 +2447,26 @@ static void mlx4_ib_scan_netdevs(struct mlx4_ib_dev *ibdev,
> >> event == NETDEV_UP || event == NETDEV_CHANGE))
> >> update_qps_port = port;
> >>
> >> + if (dev == iboe->netdevs[port - 1] &&
> >> + event == NETDEV_UP) {
> >> + struct ib_event ibev = { };
> >> +
> >> + ibev.device = &ibdev->ib_dev;
> >> + ibev.element.port_num = port;
> >> + ibev.event = IB_EVENT_PORT_ACTIVE;
> >> + ib_dispatch_event(&ibev);
> >> + }
> >> +
> >> + if (dev == iboe->netdevs[port - 1] &&
> >> + event == NETDEV_DOWN) {
> >> + struct ib_event ibev = { };
> >> +
> >> + ibev.device = &ibdev->ib_dev;
> >> + ibev.element.port_num = port;
> >> + ibev.event = IB_EVENT_PORT_ERR;
> >> + ib_dispatch_event(&ibev);
> >> + }
> >> +
> >
> > Thanks Ursula,
> >
> > I think that you need to take into account previous port states
> > (IB_PORT_DOWN vs. IB_PORT_ACTIVE).
> >
>
> Do you want to skip raising an IB event in these scenarios:
> NETDEV_UP event and previous port state is already IB_PORT_ACTIVE
> NETDEV_UP event and current port state is IB_PORT_DOWN
> NETDEV_DOWN event and previous port state is IB_PORT_DOWN
> NETDEV_DOWN event and current port state is IB_PORT_UP
>
> Isn't it guaranteed that a NETDEV_UP event is seen only when the port state has
> been IB_PORT_DOWN before (and vice versa)? At least the net/core/dev.c code makes
> sure, a NETDEV_UP event is not raised twice.
I would like to know the definitive answer too, because of I need to
clean mlx5 code from such logic or we need to add to mlx4 those checks.
>
> And even if the previous port state could be the same, would it hurt if an IB_PORT_ACTIVE
> or IB_PORT_DOWN event is raised a second time in rare cases?
I don't know, according to the code, we are relying on IB_EVENT_PORT_ERR and
IB_EVENT_PORT_ACTIVE in many places.
>
> Nevertheless I tried to determine the current port state, and found:
> If I use ib_get_cached_port_state(), I still get IB_PORT_DOWN for the NETDEV_UP event
> and vice versa. If I call ibdev->query_port() I run into locking problems, since the
> iboe->lock is held.
>
> For the net/smc code the main problem is the missing IB event in the mlx4 driver, since
> the net/smc code relies on it. If there are scenarios raising the same IB event more than
> once, this does not cause trouble.
>
> Taking into account the previous port state might be an optimization, but facing the
> problems described I am not sure if it is really worth spending extra code to maintain
> the previous and current port state.
> Do you have a sample demonstrating the urgent need for that?
>
No, I don't, but my worries about mlx4 driver are focused on preserving
(do not break mode) instead of extra features.
So, I would like to know if raising twice or more IB_EVENT_PORT_ERR and
IB_EVENT_PORT_ACTIVE is safe. If yes, let's merge it, if no, we need to
fix it.
Thanks
> > Thanks
> >
>
Attachment:
signature.asc
Description: PGP signature
