| Message ID | 23f3dca9ff4b71155bd898be1f3cbd8eeb9df27f.1436394658.git.dledford@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 7/9/2015 1:30 AM, Doug Ledford wrote: > In mlx4_main.c:do_slave_init(), the function is supposed to queue up > each work struct. However, it checks to make sure the sriov support > isn't going down first. When it is going down, it doesn't queue up the > work struct, which results in us leaking the work struct at the end of > the function. As a fix, make sure that if we don't queue up the work > struct, then we kfree it instead. > > The routine was also sub-optimal in its loop operations. Instead of > taking and releasing a spin lock over and over again, let's just take it > once, and quickly loop through what needs to be done under the spin lock > and then release it. > Hi Doug, I'd like Jack to review this before we ack, not sure if he's in today, so he might get to look on that only on Sunday. Or. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/09/2015 04:28 AM, Or Gerlitz wrote: > On 7/9/2015 1:30 AM, Doug Ledford wrote: >> In mlx4_main.c:do_slave_init(), the function is supposed to queue up >> each work struct. However, it checks to make sure the sriov support >> isn't going down first. When it is going down, it doesn't queue up the >> work struct, which results in us leaking the work struct at the end of >> the function. As a fix, make sure that if we don't queue up the work >> struct, then we kfree it instead. >> >> The routine was also sub-optimal in its loop operations. Instead of >> taking and releasing a spin lock over and over again, let's just take it >> once, and quickly loop through what needs to be done under the spin lock >> and then release it. >> > > Hi Doug, > > I'd like Jack to review this before we ack, not sure if he's in today, > so he might get to look on that only on Sunday. Try to get it reviewed before then please. If it passes my build/functional tests (which I want to get to today), it will go in a pull request tomorrow.
On 7/9/2015 3:31 PM, Doug Ledford wrote: >> i Doug, >> > >> >I'd like Jack to review this before we ack, not sure if he's in today, >> >so he might get to look on that only on Sunday. > Try to get it reviewed before then please. If it passes my > build/functional tests (which I want to get to today), it will go in a > pull request tomorrow. I hope Jack will be able to look on that -- I took a look -- seems fine. However, please (please) break it to two patches: one that fixes the leak and one that does the small optimization. Recently I came into a conclusion that each time we are violating the kernel practice of makingsure a patch has **one** logical change, we either add a bug or fix a bug in an unnoticed way, so...again, pull based on what tree/branch? Or. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/09/2015 10:02 AM, Or Gerlitz wrote: > On 7/9/2015 3:31 PM, Doug Ledford wrote: >>> i Doug, >>> > >>> >I'd like Jack to review this before we ack, not sure if he's in today, >>> >so he might get to look on that only on Sunday. >> Try to get it reviewed before then please. If it passes my >> build/functional tests (which I want to get to today), it will go in a >> pull request tomorrow. > > I hope Jack will be able to look on that -- I took a look -- seems fine. > However, please (please) break it to two patches: one that fixes the > leak and one that does the small optimization. Recently I came into a > conclusion that each time we are violating the kernel practice of > makingsure a patch has **one** logical change, we either add a bug or > fix a bug in an unnoticed way, so...again, pull based on what tree/branch? It's split. The tree will be out (probably) later today.
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c index 064454aee863..3f21a5565af2 100644 --- a/drivers/infiniband/hw/mlx4/main.c +++ b/drivers/infiniband/hw/mlx4/main.c @@ -2681,19 +2681,21 @@ static void do_slave_init(struct mlx4_ib_dev *ibdev, int slave, int do_init) kfree(dm[i]); goto out; } - } - /* initialize or tear down tunnel QPs for the slave */ - for (i = 0; i < ports; i++) { INIT_WORK(&dm[i]->work, mlx4_ib_tunnels_update_work); dm[i]->port = first_port + i + 1; dm[i]->slave = slave; dm[i]->do_init = do_init; dm[i]->dev = ibdev; - spin_lock_irqsave(&ibdev->sriov.going_down_lock, flags); - if (!ibdev->sriov.is_going_down) - queue_work(ibdev->sriov.demux[i].ud_wq, &dm[i]->work); - spin_unlock_irqrestore(&ibdev->sriov.going_down_lock, flags); } + /* initialize or tear down tunnel QPs for the slave */ + spin_lock_irqsave(&ibdev->sriov.going_down_lock, flags); + if (!ibdev->sriov.is_going_down) + for (i = 0; i < ports; i++) + queue_work(ibdev->sriov.demux[i].ud_wq, &dm[i]->work); + else + for (i = 0; i < ports; i++) + kfree(dm[i]); + spin_unlock_irqrestore(&ibdev->sriov.going_down_lock, flags); out: kfree(dm); return;
In mlx4_main.c:do_slave_init(), the function is supposed to queue up each work struct. However, it checks to make sure the sriov support isn't going down first. When it is going down, it doesn't queue up the work struct, which results in us leaking the work struct at the end of the function. As a fix, make sure that if we don't queue up the work struct, then we kfree it instead. The routine was also sub-optimal in its loop operations. Instead of taking and releasing a spin lock over and over again, let's just take it once, and quickly loop through what needs to be done under the spin lock and then release it. Signed-off-by: Doug Ledford <dledford@redhat.com> --- drivers/infiniband/hw/mlx4/main.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)