| Message ID | 20171219090956.28041-1-f.gruenbichler@proxmox.com |
|---|---|
| Headers | show |
| Series | fix skb leak in vhost_net / tun / tap (30% perf regression) | expand |
On 19.12.2017 10:09, Fabian Grünbichler wrote: > BugLink: https://bugs.launchpad.net/1738975 Bug report is either different number or private. BugLinks must point to a public bug report. -Stefan > > == SRU Justification == > > Impact: Up to 30% performance regression for traffic from hypervisor host to VM, caused by a memory leak > > Fix: Cherry-picks from upstream stable tree to fix the memory leak > > Regression Potential: Merged in 4.15 and 4.14.7, tested and verified by multiple people. > > See https://lkml.kernel.org/r/<4c7e2924-b10f-0e97-c388-c8809ecfdeeb@linux.vnet.ibm.com> for the original report and extensive discussion > See https://lkml.kernel.org/r/<1512123038-15773-1-git-send-email-wexu@redhat.com> for the patch series in question > > Wei Xu (3): > vhost: fix skb leak in handle_rx() > tap: free skb if flags error > tun: free skb in early errors > > drivers/net/tap.c | 14 ++++++++++---- > drivers/net/tun.c | 24 ++++++++++++++++++------ > drivers/vhost/net.c | 20 ++++++++++---------- > 3 files changed, 38 insertions(+), 20 deletions(-) >
On Tue, Jan 23, 2018 at 02:47:56PM +0100, Stefan Bader wrote: > On 19.12.2017 10:09, Fabian Grünbichler wrote: > > BugLink: https://bugs.launchpad.net/1738975 > > Bug report is either different number or private. BugLinks must point to a > public bug report. the links are missing a "bugs/" - should I resend both series or is this something you are willing to fixup on applying?
On 2017-12-19 10:09:53 , Fabian Grünbichler wrote: > BugLink: https://bugs.launchpad.net/1738975 > > == SRU Justification == > > Impact: Up to 30% performance regression for traffic from hypervisor host to VM, caused by a memory leak > > Fix: Cherry-picks from upstream stable tree to fix the memory leak > > Regression Potential: Merged in 4.15 and 4.14.7, tested and verified by multiple people. > > See https://lkml.kernel.org/r/<4c7e2924-b10f-0e97-c388-c8809ecfdeeb@linux.vnet.ibm.com> for the original report and extensive discussion > See https://lkml.kernel.org/r/<1512123038-15773-1-git-send-email-wexu@redhat.com> for the patch series in question > > Wei Xu (3): > vhost: fix skb leak in handle_rx() > tap: free skb if flags error > tun: free skb in early errors > > drivers/net/tap.c | 14 ++++++++++---- > drivers/net/tun.c | 24 ++++++++++++++++++------ > drivers/vhost/net.c | 20 ++++++++++---------- > 3 files changed, 38 insertions(+), 20 deletions(-) > As Stefan pointed out, the BugLink should point to a valid bug report. Also, the 'cherry picked from commit ...' line should refer to the mainline tree, however you seem to be cherry-picking from linux-stable. Please update the cherry-pick line to say 'cherry picked from commit ... linux-stable' or, preferably, just cherry-pick from mainline directly. Otherwise, the actual patches look fine. For the above reasons, NACK.
On 31.01.2018 19:44, Khaled Elmously wrote: > On 2017-12-19 10:09:53 , Fabian Grünbichler wrote: >> BugLink: https://bugs.launchpad.net/1738975 >> >> == SRU Justification == >> >> Impact: Up to 30% performance regression for traffic from hypervisor host to VM, caused by a memory leak >> >> Fix: Cherry-picks from upstream stable tree to fix the memory leak >> >> Regression Potential: Merged in 4.15 and 4.14.7, tested and verified by multiple people. >> >> See https://lkml.kernel.org/r/<4c7e2924-b10f-0e97-c388-c8809ecfdeeb@linux.vnet.ibm.com> for the original report and extensive discussion >> See https://lkml.kernel.org/r/<1512123038-15773-1-git-send-email-wexu@redhat.com> for the patch series in question >> >> Wei Xu (3): >> vhost: fix skb leak in handle_rx() >> tap: free skb if flags error >> tun: free skb in early errors >> >> drivers/net/tap.c | 14 ++++++++++---- >> drivers/net/tun.c | 24 ++++++++++++++++++------ >> drivers/vhost/net.c | 20 ++++++++++---------- >> 3 files changed, 38 insertions(+), 20 deletions(-) >> > > As Stefan pointed out, the BugLink should point to a valid bug report. Though Fabian replied since then to the list and hinted that it is just missing s .../bug/... element in the URL. I had not realized this when I went through the backlog. > > Also, the 'cherry picked from commit ...' line should refer to the mainline tree, however you seem to be cherry-picking from linux-stable. Please update the cherry-pick line to say 'cherry picked from commit ... linux-stable' or, preferably, just cherry-pick from mainline directly. There are cases when cherry-picking from a stable tree is preferable: that is if the patch had to be adapted on its way back. If the same patch was already backported for a stable tree close (in this case 4.14.y) and can be just cherry-picked from there it is less work and less prone to errors. I would add a tree reference to the reference line though. Like (cherry picked from commit 0536add671963d9875dc42f734d1608bef480dc7 linux-2.14.y) And we could do that when applying things, as long as the info is somewhere in the submission. No need to send again... well except if someone keeps sending things the wrong way after they have been told, or the reviewer had not enough coffee or is generally grumpy... ;) -Stefan -Stefan > > Otherwise, the actual patches look fine. > > For the above reasons, NACK. >
On 2018-02-01 09:29:46 , Stefan Bader wrote: > On 31.01.2018 19:44, Khaled Elmously wrote: > > On 2017-12-19 10:09:53 , Fabian Grünbichler wrote: > >> BugLink: https://bugs.launchpad.net/1738975 > >> > >> == SRU Justification == > >> > >> Impact: Up to 30% performance regression for traffic from hypervisor host to VM, caused by a memory leak > >> > >> Fix: Cherry-picks from upstream stable tree to fix the memory leak > >> > >> Regression Potential: Merged in 4.15 and 4.14.7, tested and verified by multiple people. > >> > >> See https://lkml.kernel.org/r/<4c7e2924-b10f-0e97-c388-c8809ecfdeeb@linux.vnet.ibm.com> for the original report and extensive discussion > >> See https://lkml.kernel.org/r/<1512123038-15773-1-git-send-email-wexu@redhat.com> for the patch series in question > >> > >> Wei Xu (3): > >> vhost: fix skb leak in handle_rx() > >> tap: free skb if flags error > >> tun: free skb in early errors > >> > >> drivers/net/tap.c | 14 ++++++++++---- > >> drivers/net/tun.c | 24 ++++++++++++++++++------ > >> drivers/vhost/net.c | 20 ++++++++++---------- > >> 3 files changed, 38 insertions(+), 20 deletions(-) > >> > > > > As Stefan pointed out, the BugLink should point to a valid bug report. > > Though Fabian replied since then to the list and hinted that it is just missing > s .../bug/... element in the URL. I had not realized this when I went through > the backlog. > I see it now as well, thanks. > > > > Also, the 'cherry picked from commit ...' line should refer to the mainline tree, however you seem to be cherry-picking from linux-stable. Please update the cherry-pick line to say 'cherry picked from commit ... linux-stable' or, preferably, just cherry-pick from mainline directly. > > There are cases when cherry-picking from a stable tree is preferable: that is if > the patch had to be adapted on its way back. If the same patch was already > backported for a stable tree close (in this case 4.14.y) and can be just > cherry-picked from there it is less work and less prone to errors. I would add a > tree reference to the reference line though. Like > > (cherry picked from commit 0536add671963d9875dc42f734d1608bef480dc7 linux-2.14.y) Thanks for the explanation. It seems in this case the linux-stable commit was simply cherry picked from mainline so there's no difference. But agreed, no point in resending. So this patch looks good to me now, I intend to ACK it. If you are also happy with it, please ACK it and I will apply it when I get to it in my inbox. Thanks. > > And we could do that when applying things, as long as the info is somewhere in > the submission. No need to send again... well except if someone keeps sending > things the wrong way after they have been told, or the reviewer had not enough > coffee or is generally grumpy... ;) > > -Stefan > > -Stefan > > > > Otherwise, the actual patches look fine. > > > > For the above reasons, NACK. > > > > -Khaled
On 2017-12-19 10:09:53 , Fabian Grünbichler wrote: > BugLink: https://bugs.launchpad.net/1738975 > > == SRU Justification == > > Impact: Up to 30% performance regression for traffic from hypervisor host to VM, caused by a memory leak > > Fix: Cherry-picks from upstream stable tree to fix the memory leak > > Regression Potential: Merged in 4.15 and 4.14.7, tested and verified by multiple people. > > See https://lkml.kernel.org/r/<4c7e2924-b10f-0e97-c388-c8809ecfdeeb@linux.vnet.ibm.com> for the original report and extensive discussion > See https://lkml.kernel.org/r/<1512123038-15773-1-git-send-email-wexu@redhat.com> for the patch series in question > > Wei Xu (3): > vhost: fix skb leak in handle_rx() > tap: free skb if flags error > tun: free skb in early errors > > drivers/net/tap.c | 14 ++++++++++---- > drivers/net/tun.c | 24 ++++++++++++++++++------ > drivers/vhost/net.c | 20 ++++++++++---------- > 3 files changed, 38 insertions(+), 20 deletions(-) > Acked-by: Khalid Elmously <khalid.elmously@canonical.com>
On 19.12.2017 10:09, Fabian Grünbichler wrote: > BugLink: https://bugs.launchpad.net/1738975 > > == SRU Justification == > > Impact: Up to 30% performance regression for traffic from hypervisor host to VM, caused by a memory leak > > Fix: Cherry-picks from upstream stable tree to fix the memory leak > > Regression Potential: Merged in 4.15 and 4.14.7, tested and verified by multiple people. > > See https://lkml.kernel.org/r/<4c7e2924-b10f-0e97-c388-c8809ecfdeeb@linux.vnet.ibm.com> for the original report and extensive discussion > See https://lkml.kernel.org/r/<1512123038-15773-1-git-send-email-wexu@redhat.com> for the patch series in question > > Wei Xu (3): > vhost: fix skb leak in handle_rx() > tap: free skb if flags error > tun: free skb in early errors > > drivers/net/tap.c | 14 ++++++++++---- > drivers/net/tun.c | 24 ++++++++++++++++++------ > drivers/vhost/net.c | 20 ++++++++++---------- > 3 files changed, 38 insertions(+), 20 deletions(-) > Reminder to fixup BugLink and origin of sha1 refererences. Acked-by: Stefan Bader <stefan.bader@canonical.com>
Applied On 2017-12-19 10:09:53 , Fabian Grünbichler wrote: > BugLink: https://bugs.launchpad.net/1738975 > > == SRU Justification == > > Impact: Up to 30% performance regression for traffic from hypervisor host to VM, caused by a memory leak > > Fix: Cherry-picks from upstream stable tree to fix the memory leak > > Regression Potential: Merged in 4.15 and 4.14.7, tested and verified by multiple people. > > See https://lkml.kernel.org/r/<4c7e2924-b10f-0e97-c388-c8809ecfdeeb@linux.vnet.ibm.com> for the original report and extensive discussion > See https://lkml.kernel.org/r/<1512123038-15773-1-git-send-email-wexu@redhat.com> for the patch series in question > > Wei Xu (3): > vhost: fix skb leak in handle_rx() > tap: free skb if flags error > tun: free skb in early errors > > drivers/net/tap.c | 14 ++++++++++---- > drivers/net/tun.c | 24 ++++++++++++++++++------ > drivers/vhost/net.c | 20 ++++++++++---------- > 3 files changed, 38 insertions(+), 20 deletions(-) > > -- > 2.14.2 > > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team