IPoIB fixes for 4.11

---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/infiniband/ulp/ipoib/ipoib_main.c +4 -0

index 3ce0765..a550cc6 100644
@@ -229,6 +229,10 @@ static int ipoib_change_mtu(struct net_device *dev, int new_mtu)
 
 	priv->admin_mtu = new_mtu;
 
+	if (priv->mcast_mtu < priv->admin_mtu)
+		ipoib_warn(priv, "MTU must be smaller than mcast_mtu (%u)\n",
+			   priv->mcast_mtu);
+
 	dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);
 
 	return 0;

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161228124728.26619-2-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Doug Ledford

2017-01-12 18:46 UTC #4 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 32 lines · Leon Romanovsky

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
In datagram mode, the IB UD (Unreliable Datagram) transport is used so the MTU of the interface is equal to the IB L2 MTU minus the IPoIB encapsulation header. Any request to change the MTU value above the maximum range will change the MTU to the max allowed, but will not show any warning message. An ipoib_warn is issued in such cases, letting the user know that even though the value is legal, it can't be currently applied.
Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Noa Osherovich <noaos-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/infiniband/ulp/ipoib/ipoib_main.c +0 -0
index 3ce0765..a550cc6 100644
@@ -229,6 +229,10 @@ static int ipoib_change_mtu(struct net_device
 *dev, int new_mtu)
priv->admin_mtu = new_mtu;
drivers/infiniband/ulp/ipoib/ipoib_main.c +2 -0
+	if (priv->mcast_mtu < priv->admin_mtu)
+		ipoib_warn(priv, "MTU must be smaller than mcast_mtu
(%u)\n",
drivers/infiniband/ulp/ipoib/ipoib_main.c +2 -0
+			   priv->mcast_mtu);
+

Quoted 3 lines

dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);
return 0;

I don't like this patch. First, there's no need for a warning here. That's entirely too noisy for this issue. Second, the wording of the message is poor. The user thinks they are setting the MTU, and there is no means of setting a multicast MTU, so telling them that their new MTU must be less than the mcast MTU that they can't do anything about and don't necessarily know how it is generated makes no sense. This should be no more than ipoib_dbg if we even print anything out at all, and the message should be more like "MTU must be <= the link layer MTU
- 4, use ibv_devinfo on the RDMA device to get the link layer MTU"

Signature · 4 lines

Doug Ledford <[email protected]>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

A message that is not in the archive (sent off-list or not yet received): 1484246776.123135.19.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

Leon Romanovsky

author

2017-01-12 19:35 UTC #6 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 38 lines · Doug Ledford

Quoted 32 lines · Leon Romanovsky
From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
In datagram mode, the IB UD (Unreliable Datagram) transport is used so the MTU of the interface is equal to the IB L2 MTU minus the IPoIB encapsulation header. Any request to change the MTU value above the maximum range will change the MTU to the max allowed, but will not show any warning message. An ipoib_warn is issued in such cases, letting the user know that even though the value is legal, it can't be currently applied.
Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Noa Osherovich <noaos-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/infiniband/ulp/ipoib/ipoib_main.c +0 -0
index 3ce0765..a550cc6 100644
@@ -229,6 +229,10 @@ static int ipoib_change_mtu(struct net_device
 *dev, int new_mtu)
priv->admin_mtu = new_mtu;
drivers/infiniband/ulp/ipoib/ipoib_main.c +2 -0
+	if (priv->mcast_mtu < priv->admin_mtu)
+		ipoib_warn(priv, "MTU must be smaller than mcast_mtu
(%u)\n",
drivers/infiniband/ulp/ipoib/ipoib_main.c +2 -0
+			   priv->mcast_mtu);
+
Quoted 3 lines
dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);
return 0;
I don't like this patch. First, there's no need for a warning here.

Quoted 8 lines

That's entirely too noisy for this issue. Second, the wording of the message is poor. The user thinks they are setting the MTU, and there is no means of setting a multicast MTU, so telling them that their new MTU must be less than the mcast MTU that they can't do anything about and don't necessarily know how it is generated makes no sense. This should be no more than ipoib_dbg if we even print anything out at all, and the message should be more like "MTU must be <= the link layer MTU
- 4, use ibv_devinfo on the RDMA device to get the link layer MTU"

First of all, thank you for fixing wording, for me it is the hardest part of every commit.

Second, I have a different view from you on the issue. User configured some value, which is not correct for IPoIB. In ideal world (without legacy), we were supposed to return error to him with proper message, but in our case (legacy applications) we can't (we tried and it broke some legacy ifcongfig, if I remember well). So it leaves us with one available option is to warn user about improper value.

User should know that he supplied wrong parameter.

Quoted 6 lines

--
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD

Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD

A message that is not in the archive (sent off-list or not yet received): 20170112193508.GO20392-U/[email protected]

Jason Gunthorpe

2017-01-12 20:16 UTC #8 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 7 lines · Leon Romanovsky

Quoted 7 lines
Quoted 7 lines
In datagram mode, the IB UD (Unreliable Datagram) transport is used so the MTU of the interface is equal to the IB L2 MTU minus the IPoIB encapsulation header. Any request to change the MTU value above the maximum range will change the MTU to the max allowed, but will not show any warning message. An ipoib_warn is issued in such cases, letting the user know that even though the value is legal, it can't be currently applied.

How does RC mode work then?

Quoted 6 lines

Second, I have a different view from you on the issue. User configured some value, which is not correct for IPoIB. In ideal world (without legacy), we were supposed to return error to him with proper message, but in our case (legacy applications) we can't (we tried and it broke some legacy ifcongfig, if I remember well). So it leaves us with one available option is to warn user about improper value.

This is a legitimate configuration though, the user could have a mixed MTU fabric and rely on path MTU discovery, so sets the MTU to the largest.

AFAIK the way to handle such configurations is with a multicast route that has a reduced mtu.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170112201622.GA14584-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

Leon Romanovsky

author

2017-01-13 15:08 UTC #10 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 11 lines · Jason Gunthorpe

Quoted 7 lines · Leon Romanovsky
Quoted 7 lines
Quoted 7 lines
In datagram mode, the IB UD (Unreliable Datagram) transport is used so the MTU of the interface is equal to the IB L2 MTU minus the IPoIB encapsulation header. Any request to change the MTU value above the maximum range will change the MTU to the max allowed, but will not show any warning message. An ipoib_warn is issued in such cases, letting the user know that even though the value is legal, it can't be currently applied.
How does RC mode work then?

RC mode doesn't have limitation on MTU size and it allows messages larger than IB link-layer MTU.

Quoted 8 lines

Quoted 7 lines
Second, I have a different view from you on the issue. User configured some value, which is not correct for IPoIB. In ideal world (without legacy), we were supposed to return error to him with proper message, but in our case (legacy applications) we can't (we tried and it broke some legacy ifcongfig, if I remember well). So it leaves us with one available option is to warn user about improper value.

Quoted 6 lines

This is a legitimate configuration though, the user could have a mixed MTU fabric and rely on path MTU discovery, so sets the MTU to the largest.
AFAIK the way to handle such configurations is with a multicast route that has a reduced mtu.

Multicast traffic is sent in datagram mode hence the limit will imply.

Maybe, I went too far by calling it error, but IMHO absence of indication to user of decreased MTU is not good either.

Quoted 2 lines

Jason

A message that is not in the archive (sent off-list or not yet received): 20170113150824.GQ20392-U/[email protected]

Jason Gunthorpe

2017-01-13 17:12 UTC #12 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 11 lines · Leon Romanovsky

Quoted 10 lines · Jason Gunthorpe
Quoted 7 lines · Leon Romanovsky
Quoted 7 lines
Quoted 7 lines
In datagram mode, the IB UD (Unreliable Datagram) transport is used so the MTU of the interface is equal to the IB L2 MTU minus the IPoIB encapsulation header. Any request to change the MTU value above the maximum range will change the MTU to the max allowed, but will not show any warning message. An ipoib_warn is issued in such cases, letting the user know that even though the value is legal, it can't be currently applied.

Quoted 4 lines

Quoted 1 line
How does RC mode work then?
RC mode doesn't have limitation on MTU size and it allows messages larger than IB link-layer MTU.

This is about what happens if the multicast MTU < unicast MTU - which is *exactly* the same case on RC and UD.

IPoIB cannot send a 64k multicast MTU on RC either.

Quoted 2 lines

Maybe, I went too far by calling it error, but IMHO absence of indication to user of decreased MTU is not good either.

I don't see why we should 'solve' this for UD and ignore the same problem in RC.

IMHO, the correct fix is to figure out how to limit multicast - there is something called multicast path MTU discovery - so maybe we are doing something wrong and breaking that (are we erroring too large packets inside ipoib properly?), or it is not working in the kernel or it isn't supported by the kernel ...

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170113171234.GA30551-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

Leon Romanovsky

author

2017-01-13 20:31 UTC #14 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 14 lines · Jason Gunthorpe

Quoted 13 lines · Leon Romanovsky
Quoted 11 lines · Jason Gunthorpe
Quoted 7 lines · Leon Romanovsky
Quoted 7 lines
Quoted 7 lines
In datagram mode, the IB UD (Unreliable Datagram) transport is used so the MTU of the interface is equal to the IB L2 MTU minus the IPoIB encapsulation header. Any request to change the MTU value above the maximum range will change the MTU to the max allowed, but will not show any warning message. An ipoib_warn is issued in such cases, letting the user know that even though the value is legal, it can't be currently applied.
How does RC mode work then?

Quoted 7 lines

Quoted 2 lines
RC mode doesn't have limitation on MTU size and it allows messages larger than IB link-layer MTU.
This is about what happens if the multicast MTU < unicast MTU - which is *exactly* the same case on RC and UD.
IPoIB cannot send a 64k multicast MTU on RC either.

Multicast is sent in datagram despite being in connected mode and for datagram the MTU is limited by IB

A message that is not in the archive (sent off-list or not yet received): 20170113203127.GT20392-U/[email protected]

Jason Gunthorpe

2017-01-13 21:27 UTC #16 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 7 lines · Leon Romanovsky

Quoted 4 lines
This is about what happens if the multicast MTU < unicast MTU - which is *exactly* the same case on RC and UD.
IPoIB cannot send a 64k multicast MTU on RC either.
Multicast is sent in datagram despite being in connected mode and for datagram the MTU is limited by IB

I am aware of that. Read you patch again:

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (priv->mcast_mtu < priv->admin_mtu)
+		ipoib_warn(priv, "MTU must be smaller than mcast_mtu (%u)\n",

This check is fails in RC mode as well.

And we already check if the requested MTU is beyond the port capability:

+ if (new_mtu > IPOIB_UD_MTU(priv->max_ib_mtu))
+ return -EINVAL;

So I have *no idea* why you'd want to check it against the multicast group too..

It is OK for the multicast group to have a smaller MTU than the unicast side. This is how RC operates after all.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170113212721.GB1463-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

Leon Romanovsky

author

2017-01-15 08:35 UTC #18 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 23 lines · Jason Gunthorpe

Quoted 7 lines · Leon Romanovsky
Quoted 4 lines
This is about what happens if the multicast MTU < unicast MTU - which is *exactly* the same case on RC and UD.
IPoIB cannot send a 64k multicast MTU on RC either.
Multicast is sent in datagram despite being in connected mode and for datagram the MTU is limited by IB
I am aware of that. Read you patch again:
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (priv->mcast_mtu < priv->admin_mtu)
+		ipoib_warn(priv, "MTU must be smaller than mcast_mtu (%u)\n",
This check is fails in RC mode as well.
And we already check if the requested MTU is beyond the port capability:
(quoted diff) +2 -0
+        if (new_mtu > IPOIB_UD_MTU(priv->max_ib_mtu))
+                return -EINVAL;

Quoted 3 lines

So I have *no idea* why you'd want to check it against the multicast group too..

Port MTU capability can be higher than link MTU. In this flow, the user can try to set manually MTU which will be higher than SM provided.

Immediately after that print, we have a following piece of code:
236 dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);

Quoted 4 lines

It is OK for the multicast group to have a smaller MTU than the unicast side. This is how RC operates after all.
Jason

A message that is not in the archive (sent off-list or not yet received): 20170115083543.GA20392-U/[email protected]

Jason Gunthorpe

2017-01-16 20:12 UTC #20 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 28 lines · Leon Romanovsky

Quoted 23 lines · Jason Gunthorpe
Quoted 7 lines · Leon Romanovsky
Quoted 4 lines
This is about what happens if the multicast MTU < unicast MTU - which is *exactly* the same case on RC and UD.
IPoIB cannot send a 64k multicast MTU on RC either.
Multicast is sent in datagram despite being in connected mode and for datagram the MTU is limited by IB
I am aware of that. Read you patch again:
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (priv->mcast_mtu < priv->admin_mtu)
+		ipoib_warn(priv, "MTU must be smaller than mcast_mtu (%u)\n",
This check is fails in RC mode as well.
And we already check if the requested MTU is beyond the port capability:
(quoted diff) +2 -0
+        if (new_mtu > IPOIB_UD_MTU(priv->max_ib_mtu))
+                return -EINVAL;
Quoted 3 lines
So I have *no idea* why you'd want to check it against the multicast group too..

Quoted 5 lines

Port MTU capability can be higher than link MTU. In this flow, the user can try to set manually MTU which will be higher than SM provided.
Immediately after that print, we have a following piece of code:
236 dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);

Maybe the better fix is to take that min out than generate a warning.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170116201218.GA7890-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

Leon Romanovsky

author

2017-01-17 19:31 UTC #22 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 29 lines · Jason Gunthorpe

Quoted 28 lines · Leon Romanovsky
Quoted 26 lines · Jason Gunthorpe
Quoted 7 lines · Leon Romanovsky
Quoted 4 lines
This is about what happens if the multicast MTU < unicast MTU - which is *exactly* the same case on RC and UD.
IPoIB cannot send a 64k multicast MTU on RC either.
Multicast is sent in datagram despite being in connected mode and for datagram the MTU is limited by IB
I am aware of that. Read you patch again:
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (priv->mcast_mtu < priv->admin_mtu)
+		ipoib_warn(priv, "MTU must be smaller than mcast_mtu (%u)\n",
This check is fails in RC mode as well.
And we already check if the requested MTU is beyond the port capability:
(quoted diff) +2 -0
+        if (new_mtu > IPOIB_UD_MTU(priv->max_ib_mtu))
+                return -EINVAL;
So I have *no idea* why you'd want to check it against the multicast group too..

Quoted 8 lines

Quoted 5 lines
Port MTU capability can be higher than link MTU. In this flow, the user can try to set manually MTU which will be higher than SM provided.
Immediately after that print, we have a following piece of code:
236 dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);
Maybe the better fix is to take that min out than generate a warning.

It looks like that I need more time to grasp it.
Sorry about that.

Quoted 2 lines

Jason

Leon Romanovsky

author

2017-01-19 13:12 UTC #23 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 29 lines · Jason Gunthorpe

Quoted 28 lines · Leon Romanovsky
Quoted 26 lines · Jason Gunthorpe
Quoted 7 lines · Leon Romanovsky
Quoted 4 lines
This is about what happens if the multicast MTU < unicast MTU - which is *exactly* the same case on RC and UD.
IPoIB cannot send a 64k multicast MTU on RC either.
Multicast is sent in datagram despite being in connected mode and for datagram the MTU is limited by IB
I am aware of that. Read you patch again:
Review
Quoted 2 lines
(quoted diff) +2 -0
+	if (priv->mcast_mtu < priv->admin_mtu)
+		ipoib_warn(priv, "MTU must be smaller than mcast_mtu (%u)\n",
This check is fails in RC mode as well.
And we already check if the requested MTU is beyond the port capability:
(quoted diff) +2 -0
+        if (new_mtu > IPOIB_UD_MTU(priv->max_ib_mtu))
+                return -EINVAL;
So I have *no idea* why you'd want to check it against the multicast group too..

Quoted 8 lines

Quoted 5 lines
Port MTU capability can be higher than link MTU. In this flow, the user can try to set manually MTU which will be higher than SM provided.
Immediately after that print, we have a following piece of code:
236 dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);
Maybe the better fix is to take that min out than generate a warning.

I agree with you, in theory it should work.

Let's hope that we will find such brave man who will remove this line. The removal of this one line will require from him to do extensive testing and reviewing different code paths.

Thanks

Quoted 2 lines

Jason

A message that is not in the archive (sent off-list or not yet received): 20170119131258.GT32481-U/[email protected]

Jason Gunthorpe

2017-01-19 17:33 UTC #25 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 4 lines · Leon Romanovsky

Let's hope that we will find such brave man who will remove this line. The removal of this one line will require from him to do extensive testing and reviewing different code paths.

Why? We already fully support this configuration for RC

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170119173352.GA8109-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org

Leon Romanovsky

author

2017-01-22 08:05 UTC #27 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 8 lines · Jason Gunthorpe

Quoted 4 lines · Leon Romanovsky
Let's hope that we will find such brave man who will remove this line. The removal of this one line will require from him to do extensive testing and reviewing different code paths.
Why? We already fully support this configuration for RC

At minimum, i see here one new path which should be tested. Configure small MTU -> Join multicast group -> Configure large MTU

Before removing min(..), in third step, we ensured that MTU won't exceed multicast MTU in all flows.

After removing min(..), you will have a chance of flows there MTU limit won't work.

And I just saying that it needs to be tested.

Quoted 2 lines

Jason

Doug Ledford

2017-01-12 21:58 UTC #28 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 3 lines · Leon Romanovsky

First of all, thank you for fixing wording, for me it is the hardest part of every commit.

No worries.

Quoted 5 lines

Second, I have a different view from you on the issue. User configured
some value, which is not correct for IPoIB. In ideal world (without legacy),
we were supposed to return error to him with proper message,

If you set an impossible MTU on an ethernet adapter, you get a return of EINVAL. But it doesn't mess with the kernel log at all, it's *just* EINVAL to the calling program.

Quoted 4 lines

but in our
case (legacy applications) we can't (we tried and it broke some legacy
ifcongfig, if I remember well).

ifconfig is what I used to test what Ethernet does, so it should be well and truly used to EINVAL as a return.

Quoted 2 lines

So it leaves us with one available
option is to warn user about improper value.

Even if you want to alert the user, there is no need to warn. Warn is reserved for something that is a serious error or condition, this is an EINVAL of something that is, at best, a performance issue. Unless path MTU support is broken, failing to set a larger MTU will not ever hinder actual operation. So we should never be dumping out KERN_WARN messages into the kernel log that will ping up on any root login session as well as clutter the kernel dmesg output.

Quoted 1 line

User should know that he supplied wrong parameter.

User can still find out, they just won't get proactive pings on all root sessions, instead they will have to look in the dmesg output because they are trying to figure out why their command didn't work.

Quoted 10 lines

Quoted 8 lines
--
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD

Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD

Signature · 4 lines

Doug Ledford <[email protected]>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

A message that is not in the archive (sent off-list or not yet received): 1484258308.123135.41.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

Leon Romanovsky

author

2017-01-13 15:15 UTC #30 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 17 lines · Doug Ledford

Quoted 3 lines · Leon Romanovsky

First of all, thank you for fixing wording, for me it is the hardest part of every commit.
No worries.
Quoted 5 lines
Second, I have a different view from you on the issue. User configured
some value, which is not correct for IPoIB. In ideal world (without legacy),
we were supposed to return error to him with proper message,
If you set an impossible MTU on an ethernet adapter, you get a return of EINVAL. But it doesn't mess with the kernel log at all, it's *just* EINVAL to the calling program.

Quoted 7 lines

Quoted 4 lines
but in our
case (legacy applications) we can't (we tried and it broke some legacy
ifcongfig, if I remember well).
ifconfig is what I used to test what Ethernet does, so it should be well and truly used to EINVAL as a return.

I'll recheck with Feras next week which legacy tool didn't work and will post it.

Quoted 33 lines

Quoted 3 lines
So it leaves us with one available
option is to warn user about improper value.
Even if you want to alert the user, there is no need to warn. Warn is reserved for something that is a serious error or condition, this is an EINVAL of something that is, at best, a performance issue. Unless path MTU support is broken, failing to set a larger MTU will not ever hinder actual operation. So we should never be dumping out KERN_WARN messages into the kernel log that will ping up on any root login session as well as clutter the kernel dmesg output.
Quoted 1 line
User should know that he supplied wrong parameter.
User can still find out, they just won't get proactive pings on all root sessions, instead they will have to look in the dmesg output because they are trying to figure out why their command didn't work.
Quoted 10 lines
Quoted 8 lines
--
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD

Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
--
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD

Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD

A message that is not in the archive (sent off-list or not yet received): 20170113151507.GR20392-U/[email protected]

Leon Romanovsky

author

2017-01-17 19:34 UTC #32 lore raw

Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

Quoted 23 lines · Leon Romanovsky

Quoted 22 lines · Doug Ledford
Quoted 3 lines · Leon Romanovsky

First of all, thank you for fixing wording, for me it is the hardest part of every commit.
No worries.
Quoted 5 lines
Second, I have a different view from you on the issue. User configured
some value, which is not correct for IPoIB. In ideal world (without legacy),
we were supposed to return error to him with proper message,
If you set an impossible MTU on an ethernet adapter, you get a return of EINVAL. But it doesn't mess with the kernel log at all, it's *just* EINVAL to the calling program.
Quoted 4 lines
but in our
case (legacy applications) we can't (we tried and it broke some legacy
ifcongfig, if I remember well).

Quoted 5 lines

Quoted 2 lines
ifconfig is what I used to test what Ethernet does, so it should be well and truly used to EINVAL as a return.
I'll recheck with Feras next week which legacy tool didn't work and will post it.

At the end, it was ethtool and according to Jason's comments it was correct thing do not to put return value, so we are fine.

Quoted 36 lines

Quoted 34 lines
Quoted 4 lines
So it leaves us with one available
option is to warn user about improper value.
Even if you want to alert the user, there is no need to warn. Warn is reserved for something that is a serious error or condition, this is an EINVAL of something that is, at best, a performance issue. Unless path MTU support is broken, failing to set a larger MTU will not ever hinder actual operation. So we should never be dumping out KERN_WARN messages into the kernel log that will ping up on any root login session as well as clutter the kernel dmesg output.
Quoted 1 line
User should know that he supplied wrong parameter.
User can still find out, they just won't get proactive pings on all root sessions, instead they will have to look in the dmesg output because they are trying to figure out why their command didn't work.
Quoted 10 lines
Quoted 8 lines
--
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD

Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
--
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD

Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD

Leon Romanovsky

author

2016-12-28 12:47 UTC #33 lore raw

PATCH 2/9 IB/ipoib: Set device connection mode only when needed

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

When changing the connection mode, the ipoib_set_mode function did not check if the previous connection mode equals to the new one. This commit adds the required check and return 0 if the new mode equals to the previous one.

Fixes 839fcaba355a ("IPoIB: Connected mode experimental support")

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/infiniband/ulp/ipoib/ipoib_main.c +8 -0

index a550cc6..1787f6b 100644
@@ -474,6 +474,14 @@ int ipoib_set_mode(struct net_device *dev, const char *buf)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
 
+	if ((test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags) &&
+	     !strcmp(buf, "connected\n")) ||
+	     (!test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags) &&
+	     !strcmp(buf, "datagram\n"))) {
+		ipoib_dbg(priv, "already in that mode, goes out.\n");
+		return 0;
+	}
+
 	/* flush paths if we switch modes so that connections are restarted */
 	if (IPOIB_CM_SUPPORTED(dev->dev_addr) && !strcmp(buf, "connected\n")) {
 		set_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags);

--
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161228124728.26619-3-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Doug Ledford

2017-01-12 18:47 UTC #35 lore raw

Re: [PATCH rdma-next V1 2/9] IB/ipoib: Set device connection mode only when needed

Quoted 23 lines · Leon Romanovsky

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
When changing the connection mode, the ipoib_set_mode function did not check if the previous connection mode equals to the new one. This commit adds the required check and return 0 if the new mode equals to the previous one.
Fixes 839fcaba355a ("IPoIB: Connected mode experimental support")
Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Yuval Shaia <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/infiniband/ulp/ipoib/ipoib_main.c +0 -0
index a550cc6..1787f6b 100644

drivers/infiniband/ulp/ipoib/ipoib_main.c int ipoib_set_mode(struct net_device *dev, const · line 483

Quoted 10 lines

drivers/infiniband/ulp/ipoib/ipoib_main.c +5 -0

@@ -474,6 +474,14 @@ int ipoib_set_mode(struct net_device *dev, const
 char *buf)
  {
  	struct ipoib_dev_priv *priv = netdev_priv(dev);
 
+	if ((test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags) &&
+	     !strcmp(buf, "connected\n")) ||
+	     (!test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags) &&
+	     !strcmp(buf, "datagram\n"))) {
+		ipoib_dbg(priv, "already in that mode, goes
 out.\n");

There is no need for any message here. Nothing unexpected is happening.

Signature · 4 lines

Doug Ledford <[email protected]>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

A message that is not in the archive (sent off-list or not yet received): 1484246826.123135.20.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

Leon Romanovsky

author

2017-01-12 19:25 UTC #37 lore raw

Re: [PATCH rdma-next V1 2/9] IB/ipoib: Set device connection mode only when needed

Quoted 28 lines · Doug Ledford

Quoted 27 lines · Leon Romanovsky
From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
When changing the connection mode, the ipoib_set_mode function did not check if the previous connection mode equals to the new one. This commit adds the required check and return 0 if the new mode equals to the previous one.
Fixes 839fcaba355a ("IPoIB: Connected mode experimental support")
Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Yuval Shaia <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/infiniband/ulp/ipoib/ipoib_main.c +0 -0
index a550cc6..1787f6b 100644
@@ -474,6 +474,14 @@ int ipoib_set_mode(struct net_device *dev, const
 char *buf)
  {
  	struct ipoib_dev_priv *priv = netdev_priv(dev);
 

Quoted 9 lines

drivers/infiniband/ulp/ipoib/ipoib_main.c int ipoib_set_mode(struct net_device *dev, const char *buf)
Quoted 6 lines
drivers/infiniband/ulp/ipoib/ipoib_main.c +5 -0
+	if ((test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags) &&
+	     !strcmp(buf, "connected\n")) ||
+	     (!test_bit(IPOIB_FLAG_ADMIN_CM, &priv->flags) &&
+	     !strcmp(buf, "datagram\n"))) {
+		ipoib_dbg(priv, "already in that mode, goes
 out.\n");
There is no need for any message here. Nothing unexpected is happening.

Right,
Thank you fixing it.

Quoted 6 lines

--
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD

Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD

Leon Romanovsky

author

2016-12-28 12:47 UTC #38 lore raw

PATCH 3/9 IB/ipoib: Fix deadlock over vlan_mutex

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

This patch fixes Deadlock while executing ipoib_vlan_delete.

The function takes the vlan_rwsem semaphore and calls unregister_netdevice. The later function calls ipoib_mcast_stop_thread that cause workqueue flush.

When the queue has one of the ipoib_ib_dev_flush_xxx events, a deadlock occur because these events also tries to catch the same vlan_rwsem semaphore.

To fix, unregister_netdevice should be called after releasing the semaphore.

Fixes cbbe1efa4972 ("IPoIB: Fix deadlock between ipoib_open() and child interface create")

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/infiniband/ulp/ipoib/ipoib_vlan.c +5 -1

index fd81111..9682183 100644
@@ -196,7 +196,6 @@ int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey)
 	list_for_each_entry_safe(priv, tpriv, &ppriv->child_intfs, list) {
 		if (priv->pkey == pkey &&
 		    priv->child_type == IPOIB_LEGACY_CHILD) {
-			unregister_netdevice(priv->dev);
 			list_del(&priv->list);
 			dev = priv->dev;
 			break;

@@ -204,6 +203,11 @@ int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey)
 	}
 	up_write(&ppriv->vlan_rwsem);
 
+	if (dev) {
+		ipoib_dbg(ppriv, "delete child vlan %s\n", dev->name);
+		unregister_netdevice(dev);
+	}
+
 	rtnl_unlock();
 
 	if (dev) {

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-28 12:47 UTC #39 lore raw

PATCH 4/9 IB/ipoib: Fix deadlock between rmmod and set_mode

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

When calling set_mode from sys/fs, the call flow locks the sys/fs lock first and then tries to lock rtnl_lock (when calling ipoib_set_mod). On the other hand, the rmmod call flow takes the rtnl_lock first (when calling unregister_netdev) and then tries to take the sys/fs lock. Deadlock a->b, b->a.

The problem starts when ipoib_set_mod frees it's rtnl_lck and tries to get it after that.

    set_mod:
    [<ffffffff8104f2bd>] ? check_preempt_curr+0x6d/0x90
    [<ffffffff814fee8e>] __mutex_lock_slowpath+0x13e/0x180
    [<ffffffff81448655>] ? __rtnl_unlock+0x15/0x20
    [<ffffffff814fed2b>] mutex_lock+0x2b/0x50
    [<ffffffff81448675>] rtnl_lock+0x15/0x20
    [<ffffffffa02ad807>] ipoib_set_mode+0x97/0x160 [ib_ipoib]
    [<ffffffffa02b5f5b>] set_mode+0x3b/0x80 [ib_ipoib]
    [<ffffffff8134b840>] dev_attr_store+0x20/0x30
    [<ffffffff811f0fe5>] sysfs_write_file+0xe5/0x170
    [<ffffffff8117b068>] vfs_write+0xb8/0x1a0
    [<ffffffff8117ba81>] sys_write+0x51/0x90
    [<ffffffff8100b0f2>] system_call_fastpath+0x16/0x1b

    rmmod:
    [<ffffffff81279ffc>] ? put_dec+0x10c/0x110
    [<ffffffff8127a2ee>] ? number+0x2ee/0x320
    [<ffffffff814fe6a5>] schedule_timeout+0x215/0x2e0
    [<ffffffff8127cc04>] ? vsnprintf+0x484/0x5f0
    [<ffffffff8127b550>] ? string+0x40/0x100
    [<ffffffff814fe323>] wait_for_common+0x123/0x180
    [<ffffffff81060250>] ? default_wake_function+0x0/0x20
    [<ffffffff8119661e>] ? ifind_fast+0x5e/0xb0
    [<ffffffff814fe43d>] wait_for_completion+0x1d/0x20
    [<ffffffff811f2e68>] sysfs_addrm_finish+0x228/0x270
    [<ffffffff811f2fb3>] sysfs_remove_dir+0xa3/0xf0
    [<ffffffff81273f66>] kobject_del+0x16/0x40
    [<ffffffff8134cd14>] device_del+0x184/0x1e0
    [<ffffffff8144e59b>] netdev_unregister_kobject+0xab/0xc0
    [<ffffffff8143c05e>] rollback_registered+0xae/0x130
    [<ffffffff8143c102>] unregister_netdevice+0x22/0x70
    [<ffffffff8143c16e>] unregister_netdev+0x1e/0x30
    [<ffffffffa02a91b0>] ipoib_remove_one+0xe0/0x120 [ib_ipoib]
    [<ffffffffa01ed95f>] ib_unregister_device+0x4f/0x100 [ib_core]
    [<ffffffffa021f5e1>] mlx4_ib_remove+0x41/0x180 [mlx4_ib]
    [<ffffffffa01ab771>] mlx4_remove_device+0x71/0x90 [mlx4_core]

Fixes 862096a8bbf8 ("IB/ipoib: Add more rtnl_link_ops callbacks")

Cc <[email protected]> # v3.6+

Cc Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c   | 12 +++++++-----
 drivers/infiniband/ulp/ipoib/ipoib_main.c |  6 ++----
 2 files changed, 9 insertions(+), 9 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_cm.c +7 -5

index 096c4f6..1c7a9a1 100644
@@ -1507,12 +1507,14 @@ static ssize_t set_mode(struct device *d, struct device_attribute *attr,
 
 	ret = ipoib_set_mode(dev, buf);
 
-	rtnl_unlock();
-
-	if (!ret)
-		return count;
+	/* The assumption is that the function ipoib_set_mode returned
+	 * with the rtnl held by it, if not the value -EBUSY returned,
+	 * then no need to rtnl_unlock
+	 */
+	if (ret != -EBUSY)
+		rtnl_unlock();
 
-	return ret;
+	return (!ret || ret == -EBUSY) ? count : ret;
 }
 
 static DEVICE_ATTR(mode, S_IWUSR | S_IRUGO, show_mode, set_mode);

drivers/infiniband/ulp/ipoib/ipoib_main.c +2 -4

index 1787f6b..1090fe2 100644
@@ -493,8 +493,7 @@ int ipoib_set_mode(struct net_device *dev, const char *buf)
 		priv->tx_wr.wr.send_flags &= ~IB_SEND_IP_CSUM;
 
 		ipoib_flush_paths(dev);
-		rtnl_lock();
-		return 0;
+		return (!rtnl_trylock()) ? -EBUSY : 0;
 	}
 
 	if (!strcmp(buf, "datagram\n")) {

@@ -503,8 +502,7 @@ int ipoib_set_mode(struct net_device *dev, const char *buf)
 		dev_set_mtu(dev, min(priv->mcast_mtu, dev->mtu));
 		rtnl_unlock();
 		ipoib_flush_paths(dev);
-		rtnl_lock();
-		return 0;
+		return (!rtnl_trylock()) ? -EBUSY : 0;
 	}
 
 	return -EINVAL;

--
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-28 12:47 UTC #40 lore raw

PATCH 5/9 IB/ipoib: rtnl_unlock can not come after free_netdev

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

The ipoib_vlan_add function calls rtnl_unlock after free_netdev, rtnl_unlock not only releases the lock, but also calls netdev_run_todo. The latter function browses the net_todo_list array and completes the unregistration of all its net_device instances. If we call free_netdev before rtnl_unlock, then netdev_run_todo call over the freed device causes panic.
To fix, move rtnl_unlock call before free_netdev call.

Fixes 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")

Cc Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_vlan.c +2 -2

index 9682183..d9dab4a 100644
@@ -168,11 +168,11 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
 out:
 	up_write(&ppriv->vlan_rwsem);
 
+	rtnl_unlock();
+
 	if (result)
 		free_netdev(priv->dev);
 
-	rtnl_unlock();
-
 	return result;
 }

--
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161228124728.26619-6-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Or Gerlitz

2016-12-29 20:55 UTC #42 lore raw

Re: [PATCH rdma-next V1 5/9] IB/ipoib: rtnl_unlock can not come after free_netdev

Quoted 8 lines · Leon Romanovsky

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
The ipoib_vlan_add function calls rtnl_unlock after free_netdev, rtnl_unlock not only releases the lock, but also calls netdev_run_todo. The latter function browses the net_todo_list array and completes the unregistration of all its net_device instances. If we call free_netdev before rtnl_unlock, then netdev_run_todo call over the freed device causes panic.

RU claiming that we are crashing 100.0% here? since when? ever?

Quoted 12 lines

To fix, move rtnl_unlock call before free_netdev call.
Fixes 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
Cc Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/[email protected]>
drivers/infiniband/ulp/ipoib/ipoib_vlan.c +0 -0
index 9682183..d9dab4a 100644
@@ -168,11 +168,11 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
 out:
        up_write(&ppriv->vlan_rwsem);

drivers/infiniband/ulp/ipoib/ipoib_vlan.c int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey) · line 177

Quoted 10 lines

drivers/infiniband/ulp/ipoib/ipoib_vlan.c +2 -2

 
+       rtnl_unlock();
+
        if (result)
                free_netdev(priv->dev);
 
-       rtnl_unlock();
-
        return result;
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CAJ3xEMg+UuvUqqaFrBXa+S+S2qLg_FowZLrwvhYzaB7JTkn=pQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Leon Romanovsky

2017-01-01 06:39 UTC #44 lore raw

Re: [PATCH rdma-next V1 5/9] IB/ipoib: rtnl_unlock can not come after free_netdev

Quoted 11 lines · Or Gerlitz

Quoted 8 lines · Leon Romanovsky
From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
The ipoib_vlan_add function calls rtnl_unlock after free_netdev, rtnl_unlock not only releases the lock, but also calls netdev_run_todo. The latter function browses the net_todo_list array and completes the unregistration of all its net_device instances. If we call free_netdev before rtnl_unlock, then netdev_run_todo call over the freed device causes panic.

Quoted 1 line

RU claiming that we are crashing 100.0% here? since when? ever?

According to our internal bugtracker (see #131352), this bug was literaly forever (4 years old), and it was fixed internaly in MOFED 2.0.

Quoted 27 lines

Quoted 13 lines
To fix, move rtnl_unlock call before free_netdev call.
Fixes 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
Cc Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/[email protected]>
drivers/infiniband/ulp/ipoib/ipoib_vlan.c +0 -0
index 9682183..d9dab4a 100644
@@ -168,11 +168,11 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)
 out:
        up_write(&ppriv->vlan_rwsem);
drivers/infiniband/ulp/ipoib/ipoib_vlan.c int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey) · line 177
Quoted 10 lines
drivers/infiniband/ulp/ipoib/ipoib_vlan.c +2 -2
 
+       rtnl_unlock();
+
        if (result)
                free_netdev(priv->dev);
 
-       rtnl_unlock();
-
        return result;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170101063945.GP26885-U/[email protected]

Or Gerlitz

2017-01-01 07:19 UTC #46 lore raw

Re: [PATCH rdma-next V1 5/9] IB/ipoib: rtnl_unlock can not come after free_netdev

Quoted 12 lines · Leon Romanovsky

Quoted 11 lines · Or Gerlitz
Quoted 8 lines · Leon Romanovsky
From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
The ipoib_vlan_add function calls rtnl_unlock after free_netdev, rtnl_unlock not only releases the lock, but also calls netdev_run_todo. The latter function browses the net_todo_list array and completes the unregistration of all its net_device instances. If we call free_netdev before rtnl_unlock, then netdev_run_todo call over the freed device causes panic.

Quoted 5 lines

Quoted 1 line
RU claiming that we are crashing 100.0% here? since when? ever?
According to our internal bugtracker (see #131352), this bug was literaly forever (4 years old), and it was fixed internaly in MOFED 2.0.

Did you try it out? e.g to see that this crashes 100% as you claim?

I am pretty much sure this is not correct, and I'd like to see Erez commenting on that and supporting your claim.

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
To fix, move rtnl_unlock call before free_netdev call.
Fixes 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
Cc Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CAJ3xEMj-6aLraLqH+VBeyqTivSnB0hRo_Hzaiy_LChX8yXDjVg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Erez Shitrit

2017-01-01 07:45 UTC #48 lore raw

Re: [PATCH rdma-next V1 5/9] IB/ipoib: rtnl_unlock can not come after free_netdev

Quoted 15 lines · Or Gerlitz

Quoted 14 lines · Leon Romanovsky
Quoted 12 lines · Or Gerlitz
Quoted 8 lines · Leon Romanovsky
From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
The ipoib_vlan_add function calls rtnl_unlock after free_netdev, rtnl_unlock not only releases the lock, but also calls netdev_run_todo. The latter function browses the net_todo_list array and completes the unregistration of all its net_device instances. If we call free_netdev before rtnl_unlock, then netdev_run_todo call over the freed device causes panic.
RU claiming that we are crashing 100.0% here? since when? ever?

Quoted 9 lines

Quoted 3 lines
According to our internal bugtracker (see #131352), this bug was literaly forever (4 years old), and it was fixed internaly in MOFED 2.0.
Did you try it out? e.g to see that this crashes 100% as you claim?
I am pretty much sure this is not correct, and I'd like to see Erez commenting on that and supporting your claim.

It happens in error flow only, when the call for __ipoib_vlan_add returns with error, in that case we will call free_netdev before rtnl_unlock.

The fix is similar to what we have in ipoib_vlan_delete function:

         ....
         rtnl_unlock();

         if (dev) {
                 free_netdev(dev);
                 return 0;
         }

First rtnl_unlock() and then free_netdev

Thanks, Erez

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
To fix, move rtnl_unlock call before free_netdev call.
Fixes 9baa0b036410 ("IB/ipoib: Add rtnl_link_ops support")
Cc Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-28 12:47 UTC #49 lore raw

PATCH 6/9 IB/ipoib: Add detailed error message to dev_queue_xmit call

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Add a detailed return code to dev_queue_xmit function when calling to requeue packet via __skb_dequeue.

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c        | 7 ++++---
 drivers/infiniband/ulp/ipoib/ipoib_main.c      | 8 +++++---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 6 ++++--
 3 files changed, 13 insertions(+), 8 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_cm.c +4 -3

index 1c7a9a1..a720d2d 100644
@@ -1015,9 +1015,10 @@ static int ipoib_cm_rep_handler(struct ib_cm_id *cm_id, struct ib_cm_event *even
 
 	while ((skb = __skb_dequeue(&skqueue))) {
 		skb->dev = p->dev;
-		if (dev_queue_xmit(skb))
-			ipoib_warn(priv, "dev_queue_xmit failed "
-				   "to requeue packet\n");
+		ret = dev_queue_xmit(skb);
+		if (ret)
+			ipoib_warn(priv, "%s:dev_queue_xmit failed to re-queue packet, ret:%d\n",
+				   __func__, ret);
 	}
 
 	ret = ib_send_cm_rtu(cm_id, NULL, 0);

drivers/infiniband/ulp/ipoib/ipoib_main.c +5 -3

index 1090fe2..b5e1e4d 100644
@@ -844,10 +844,12 @@ static void path_rec_completion(int status,
 		ipoib_put_ah(old_ah);
 
 	while ((skb = __skb_dequeue(&skqueue))) {
+		int ret;
 		skb->dev = dev;
-		if (dev_queue_xmit(skb))
-			ipoib_warn(priv, "dev_queue_xmit failed "
-				   "to requeue packet\n");
+		ret = dev_queue_xmit(skb);
+		if (ret)
+			ipoib_warn(priv, "%s: dev_queue_xmit failed to re-queue packet, ret:%d\n",
+				   __func__, ret);
 	}
 }

drivers/infiniband/ulp/ipoib/ipoib_multicast.c +4 -2

index fddff40..7c6c67b 100644
@@ -314,9 +314,11 @@ static int ipoib_mcast_join_finish(struct ipoib_mcast *mcast,
 		netif_tx_unlock_bh(dev);
 
 		skb->dev = dev;
-		if (dev_queue_xmit(skb))
-			ipoib_warn(priv, "dev_queue_xmit failed to requeue packet\n");
 
+		ret = dev_queue_xmit(skb);
+		if (ret)
+			ipoib_warn(priv, "%s:dev_queue_xmit failed to re-queue packet, ret:%d\n",
+				   __func__, ret);
 		netif_tx_lock_bh(dev);
 	}
 	netif_tx_unlock_bh(dev);

--
2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-28 12:47 UTC #50 lore raw

PATCH 7/9 IB/ipoib: Use debug prints instead of warnings in RNR WC status

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

If a receive request has not been posted to the work queue, the incoming message is rejected and the peer will receive a receiver-not-ready (RNR) error. In IPoIB, IB_WC_RNR_RETRY_EXC_ERR error is part of the life cycle therefore ipoib_cm_handle_tx_wc function will print to debug instead of warnings.

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_cm.c +6 -3

index a720d2d..c433e72 100644
@@ -820,9 +820,12 @@ void ipoib_cm_handle_tx_wc(struct net_device *dev, struct ib_wc *wc)
 	    wc->status != IB_WC_WR_FLUSH_ERR) {
 		struct ipoib_neigh *neigh;
 
-		ipoib_dbg(priv, "failed cm send event "
-			   "(status=%d, wrid=%d vend_err %x)\n",
-			   wc->status, wr_id, wc->vendor_err);
+		if (wc->status != IB_WC_RNR_RETRY_EXC_ERR)
+			ipoib_warn(priv, "failed cm send event (status=%d, wrid=%d vend_err %x)\n",
+				   wc->status, wr_id, wc->vendor_err);
+		else
+			ipoib_dbg(priv, "failed cm send event (status=%d, wrid=%d vend_err %x)\n",
+				  wc->status, wr_id, wc->vendor_err);
 
 		spin_lock_irqsave(&priv->lock, flags);
 		neigh = tx->neigh;

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161228124728.26619-8-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Yuval Shaia

Reviewed-by

2016-12-28 13:11 UTC #52 lore raw

Re: [PATCH rdma-next V1 7/9] IB/ipoib: Use debug prints instead of warnings in RNR WC status

Quoted 25 lines · Leon Romanovsky

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
If a receive request has not been posted to the work queue, the incoming message is rejected and the peer will receive a receiver-not-ready (RNR) error. In IPoIB, IB_WC_RNR_RETRY_EXC_ERR error is part of the life cycle therefore ipoib_cm_handle_tx_wc function will print to debug instead of warnings.
Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_cm.c +0 -2
index a720d2d..c433e72 100644
@@ -820,9 +820,12 @@ void ipoib_cm_handle_tx_wc(struct net_device *dev, struct ib_wc *wc)
 	    wc->status != IB_WC_WR_FLUSH_ERR) {
 		struct ipoib_neigh *neigh;
 
-		ipoib_dbg(priv, "failed cm send event "
-			   "(status=%d, wrid=%d vend_err %x)\n",

drivers/infiniband/ulp/ipoib/ipoib_cm.c void ipoib_cm_handle_tx_wc(struct net_device *dev, struct ib_wc *wc) · line 831

Quoted 10 lines

drivers/infiniband/ulp/ipoib/ipoib_cm.c +6 -1

-			   wc->status, wr_id, wc->vendor_err);
+		if (wc->status != IB_WC_RNR_RETRY_EXC_ERR)
+			ipoib_warn(priv, "failed cm send event (status=%d, wrid=%d vend_err %x)\n",
+				   wc->status, wr_id, wc->vendor_err);
+		else
+			ipoib_dbg(priv, "failed cm send event (status=%d, wrid=%d vend_err %x)\n",
+				  wc->status, wr_id, wc->vendor_err);
 
 		spin_lock_irqsave(&priv->lock, flags);
 		neigh = tx->neigh;

Quoted 7 lines

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2016-12-28 12:47 UTC #53 lore raw

PATCH 8/9 IB/ipoib: Replace list_del of the neigh->list with list_del_init

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

In order to resolve a situation where a few process delete the same list element in sequence and cause panic, list_del is replaced with list_del_init. In this case if the first process that calls list_del releases the lock before acquiring it again, other processes who can acquire the lock will call list_del_init.

Fixes b63b70d87741 ("IPoIB: Use a private hash table for path lookup")

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_main.c +4 -4

index b5e1e4d..d8af197 100644
@@ -1298,7 +1298,7 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 						   rcu_dereference_protected(neigh->hnext,
 									     lockdep_is_held(&priv->lock)));
 				/* remove from path/mc list */
-				list_del(&neigh->list);
+				list_del_init(&neigh->list);
 				call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 			} else {
 				np = &neigh->hnext;

@@ -1462,7 +1462,7 @@ void ipoib_neigh_free(struct ipoib_neigh *neigh)
 					   rcu_dereference_protected(neigh->hnext,
 								     lockdep_is_held(&priv->lock)));
 			/* remove from parent list */
-			list_del(&neigh->list);
+			list_del_init(&neigh->list);
 			call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 			return;
 		} else {

@@ -1547,7 +1547,7 @@ void ipoib_del_neighs_by_gid(struct net_device *dev, u8 *gid)
 						   rcu_dereference_protected(neigh->hnext,
 									     lockdep_is_held(&priv->lock)));
 				/* remove from parent list */
-				list_del(&neigh->list);
+				list_del_init(&neigh->list);
 				call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 			} else {
 				np = &neigh->hnext;

@@ -1589,7 +1589,7 @@ static void ipoib_flush_neighs(struct ipoib_dev_priv *priv)
 					   rcu_dereference_protected(neigh->hnext,
 								     lockdep_is_held(&priv->lock)));
 			/* remove from path/mc list */
-			list_del(&neigh->list);
+			list_del_init(&neigh->list);
 			call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 		}
 	}

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161228124728.26619-9-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Yuval Shaia

Reviewed-by

2017-01-02 12:29 UTC #55 lore raw

Re: [PATCH rdma-next V1 8/9] IB/ipoib: Replace list_del of the neigh->list with list_del_init

Quoted 1 line · Leon Romanovsky

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Any reason why this patch, out of 3 in this patch set uses "Replace" instead of "Change" in commit header?

Quoted 64 lines

Fixes b63b70d87741 ("IPoIB: Use a private hash table for path lookup")

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_main.c +4 -4

index b5e1e4d..d8af197 100644
@@ -1298,7 +1298,7 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)
 						   rcu_dereference_protected(neigh->hnext,
 									     lockdep_is_held(&priv->lock)));
 				/* remove from path/mc list */
-				list_del(&neigh->list);
+				list_del_init(&neigh->list);
 				call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 			} else {
 				np = &neigh->hnext;

@@ -1462,7 +1462,7 @@ void ipoib_neigh_free(struct ipoib_neigh *neigh)
 					   rcu_dereference_protected(neigh->hnext,
 								     lockdep_is_held(&priv->lock)));
 			/* remove from parent list */
-			list_del(&neigh->list);
+			list_del_init(&neigh->list);
 			call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 			return;
 		} else {

@@ -1547,7 +1547,7 @@ void ipoib_del_neighs_by_gid(struct net_device *dev, u8 *gid)
 						   rcu_dereference_protected(neigh->hnext,
 									     lockdep_is_held(&priv->lock)));
 				/* remove from parent list */
-				list_del(&neigh->list);
+				list_del_init(&neigh->list);
 				call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 			} else {
 				np = &neigh->hnext;

@@ -1589,7 +1589,7 @@ static void ipoib_flush_neighs(struct ipoib_dev_priv *priv)
 					   rcu_dereference_protected(neigh->hnext,
 								     lockdep_is_held(&priv->lock)));
 			/* remove from path/mc list */
-			list_del(&neigh->list);
+			list_del_init(&neigh->list);
 			call_rcu(&neigh->rcu, ipoib_neigh_reclaim);
 		}
 	}

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170102122917.GB25669-Hxa29pjIrETlQW142y8m19+IiqhCXseY@public.gmane.org

Leon Romanovsky

author

2017-01-02 18:19 UTC #57 lore raw

Re: [PATCH rdma-next V1 8/9] IB/ipoib: Replace list_del of the neigh->list with list_del_init

Quoted 5 lines · Yuval Shaia

Quoted 1 line · Leon Romanovsky
From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Any reason why this patch, out of 3 in this patch set uses "Replace" instead of "Change" in commit header?

No real reason.

Quoted 2 lines

Reviewed-by Yuval Shaia <[email protected]>

Thank you for your reviewes.

Leon Romanovsky

author

2016-12-28 12:47 UTC #58 lore raw

PATCH 9/9 IB/ipoib: Change list_del to list_del_init in the tx object

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Since ipoib_cm_tx_start function and ipoib_cm_tx_reap function belong to different work queues, they can run in parallel.
In this case if ipoib_cm_tx_reap calls list_del and release the lock, ipoib_cm_tx_start may acquire it and call list_del_init on the already deleted object.
Changing list_del to list_del_init in ipoib_cm_tx_reap fixes the problem.

Fixes 839fcaba355a ("IPoIB: Connected mode experimental support")

Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/infiniband/ulp/ipoib/ipoib_cm.c +1 -1

index c433e72..53d69d7 100644
@@ -1392,7 +1392,7 @@ static void ipoib_cm_tx_reap(struct work_struct *work)
 
 	while (!list_empty(&priv->cm.reap_list)) {
 		p = list_entry(priv->cm.reap_list.next, typeof(*p), list);
-		list_del(&p->list);
+		list_del_init(&p->list);
 		spin_unlock_irqrestore(&priv->lock, flags);
 		netif_tx_unlock_bh(dev);
 		ipoib_cm_tx_destroy(p);

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20161228124728.26619-10-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Yuval Shaia

Reviewed-by

2017-01-02 12:21 UTC #60 lore raw

Re: [PATCH rdma-next V1 9/9] IB/ipoib: Change list_del to list_del_init in the tx object

Quoted 23 lines · Leon Romanovsky

From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Since ipoib_cm_tx_start function and ipoib_cm_tx_reap function belong to different work queues, they can run in parallel.
In this case if ipoib_cm_tx_reap calls list_del and release the lock, ipoib_cm_tx_start may acquire it and call list_del_init on the already deleted object.
Changing list_del to list_del_init in ipoib_cm_tx_reap fixes the problem.
Fixes 839fcaba355a ("IPoIB: Connected mode experimental support")
Signed-off-by Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>
Reviewed-by Alex Vesker <valex-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/ulp/ipoib/ipoib_cm.c +0 -0
index c433e72..53d69d7 100644

drivers/infiniband/ulp/ipoib/ipoib_cm.c static void ipoib_cm_tx_reap(struct work_struct *work) · line 1398

Quoted 8 lines

drivers/infiniband/ulp/ipoib/ipoib_cm.c +1 -1

@@ -1392,7 +1392,7 @@ static void ipoib_cm_tx_reap(struct work_struct *work)
 
 	while (!list_empty(&priv->cm.reap_list)) {
 		p = list_entry(priv->cm.reap_list.next, typeof(*p), list);
-		list_del(&p->list);
+		list_del_init(&p->list);
 		spin_unlock_irqrestore(&priv->lock, flags);
 		netif_tx_unlock_bh(dev);
 		ipoib_cm_tx_destroy(p);

Quoted 7 lines

Signature · 6 lines

2.10.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Doug Ledford

2017-01-12 19:02 UTC #61 lore raw