* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Boris Ostrovsky @ 2017-10-17 13:58 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Juergen Gross, Mike Galbraith, Ingo Molnar, Peter Zijlstra,
Andrew Cooper, Rusty Russell, virtualization, x86, linux-kernel,
Chris Wright, live-patching, Linus Torvalds, Borislav Petkov,
Andy Lutomirski, H. Peter Anvin, xen-devel, Thomas Gleixner,
Sasha Levin, Jiri Slaby, Alok Kataria
In-Reply-To: <20171017052413.nzbqniurzw7eim4b@treble>
On 10/17/2017 01:24 AM, Josh Poimboeuf wrote:
> On Mon, Oct 16, 2017 at 02:18:48PM -0400, Boris Ostrovsky wrote:
>> On 10/12/2017 03:53 PM, Boris Ostrovsky wrote:
>>> On 10/12/2017 03:27 PM, Andrew Cooper wrote:
>>>> On 12/10/17 20:11, Boris Ostrovsky wrote:
>>>>> There is also another problem:
>>>>>
>>>>> [ 1.312425] general protection fault: 0000 [#1] SMP
>>>>> [ 1.312901] Modules linked in:
>>>>> [ 1.313389] CPU: 0 PID: 1 Comm: init Not tainted 4.14.0-rc4+ #6
>>>>> [ 1.313878] task: ffff88003e2c0000 task.stack: ffffc9000038c000
>>>>> [ 1.314360] RIP: 10000e030:entry_SYSCALL_64_fastpath+0x1/0xa5
>>>>> [ 1.314854] RSP: e02b:ffffc9000038ff50 EFLAGS: 00010046
>>>>> [ 1.315336] RAX: 000000000000000c RBX: 000055f550168040 RCX:
>>>>> 00007fcfc959f59a
>>>>> [ 1.315827] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
>>>>> 0000000000000000
>>>>> [ 1.316315] RBP: 000000000000000a R08: 000000000000037f R09:
>>>>> 0000000000000064
>>>>> [ 1.316805] R10: 000000001f89cbf5 R11: ffff88003e2c0000 R12:
>>>>> 00007fcfc958ad60
>>>>> [ 1.317300] R13: 0000000000000000 R14: 000055f550185954 R15:
>>>>> 0000000000001000
>>>>> [ 1.317801] FS: 0000000000000000(0000) GS:ffff88003f800000(0000)
>>>>> knlGS:0000000000000000
>>>>> [ 1.318267] CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>> [ 1.318750] CR2: 00007fcfc97ab218 CR3: 000000003c88e000 CR4:
>>>>> 0000000000042660
>>>>> [ 1.319235] Call Trace:
>>>>> [ 1.319700] Code: 51 50 57 56 52 51 6a da 41 50 41 51 41 52 41 53 48
>>>>> 83 ec 30 65 4c 8b 1c 25 c0 d2 00 00 41 f7 03 df 39 08 90 0f 85 a5 00 00
>>>>> 00 50 <ff> 15 9c 95 d0 ff 58 48 3d 4c 01 00 00 77 0f 4c 89 d1 ff 14 c5
>>>>> [ 1.321161] RIP: entry_SYSCALL_64_fastpath+0x1/0xa5 RSP: ffffc9000038ff50
>>>>> [ 1.344255] ---[ end trace d7cb8cd6cd7c294c ]---
>>>>> [ 1.345009] Kernel panic - not syncing: Attempted to kill init!
>>>>> exitcode=0x0000000b
>>>>>
>>>>>
>>>>> All code
>>>>> ========
>>>>> 0: 51 push %rcx
>>>>> 1: 50 push %rax
>>>>> 2: 57 push %rdi
>>>>> 3: 56 push %rsi
>>>>> 4: 52 push %rdx
>>>>> 5: 51 push %rcx
>>>>> 6: 6a da pushq $0xffffffffffffffda
>>>>> 8: 41 50 push %r8
>>>>> a: 41 51 push %r9
>>>>> c: 41 52 push %r10
>>>>> e: 41 53 push %r11
>>>>> 10: 48 83 ec 30 sub $0x30,%rsp
>>>>> 14: 65 4c 8b 1c 25 c0 d2 mov %gs:0xd2c0,%r11
>>>>> 1b: 00 00
>>>>> 1d: 41 f7 03 df 39 08 90 testl $0x900839df,(%r11)
>>>>> 24: 0f 85 a5 00 00 00 jne 0xcf
>>>>> 2a: 50 push %rax
>>>>> 2b:* ff 15 9c 95 d0 ff callq *-0x2f6a64(%rip) #
>>>>> 0xffffffffffd095cd <-- trapping instruction
>>>>> 31: 58 pop %rax
>>>>> 32: 48 3d 4c 01 00 00 cmp $0x14c,%rax
>>>>> 38: 77 0f ja 0x49
>>>>> 3a: 4c 89 d1 mov %r10,%rcx
>>>>> 3d: ff .byte 0xff
>>>>> 3e: 14 c5 adc $0xc5,%al
>>>>>
>>>>>
>>>>> so the original 'cli' was replaced with the pv call but to me the offset
>>>>> looks a bit off, no? Shouldn't it always be positive?
>>>> callq takes a 32bit signed displacement, so jumping back by up to 2G is
>>>> perfectly legitimate.
>>> Yes, but
>>>
>>> ostr@workbase> nm vmlinux | grep entry_SYSCALL_64_fastpath
>>> ffffffff817365dd t entry_SYSCALL_64_fastpath
>>> ostr@workbase> nm vmlinux | grep " pv_irq_ops"
>>> ffffffff81c2dbc0 D pv_irq_ops
>>> ostr@workbase>
>>>
>>> so pv_irq_ops.irq_disable is about 5MB ahead of where we are now. (I
>>> didn't mean that x86 instruction set doesn't allow negative
>>> displacement, I was trying to say that pv_irq_ops always live further down)
>> I believe the problem is this:
>>
>> #define PV_INDIRECT(addr) *addr(%rip)
>>
>> The displacement that the linker computes will be relative to the where
>> this instruction is placed at the time of linking, which is in
>> .pv_altinstructions (and not .text). So when we copy it into .text the
>> displacement becomes bogus.
> apply_alternatives() is supposed to adjust that displacement based on
> the new IP, though it could be messing that up somehow. (See patch
> 10/13.)
>
That patch doesn't take into account the fact that replacement
instructions may have to save/restore registers. So, for example,
- if (a->replacementlen && is_jmp(replacement[0]))
+ } else if (a->replacementlen == 6 && *insnbuf == 0xff &&
+ *(insnbuf+1) == 0x15) {
+ /* indirect call */
+ *(s32 *)(insnbuf + 2) += replacement - instr;
+ DPRINTK("Fix indirect CALL offset: 0x%x, CALL *0x%lx",
+ *(s32 *)(insnbuf + 2),
+ (unsigned long)instr + *(s32 *)(insnbuf + 2) + 6);
+
doesn't do the adjustment of
2a: 50 push %rax
2b:* ff 15 9c 95 d0 ff callq *-0x2f6a64(%rip)
31: 58 pop %rax
because instbuf points to 'push' and not to 'call'.
-boris
^ permalink raw reply
* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Brian Gerst @ 2017-10-17 13:10 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Juergen Gross, Mike Galbraith, H. Peter Anvin, Peter Zijlstra,
Andrew Cooper, Rusty Russell, Linux Virtualization,
the arch/x86 maintainers, Linux Kernel Mailing List, Chris Wright,
live-patching, Linus Torvalds, Borislav Petkov, Andy Lutomirski,
Josh Poimboeuf, Ingo Molnar, xen-devel, Thomas Gleixner,
Sasha Levin, Jiri Slaby, Alok Kataria
In-Reply-To: <b9fdc3ec-87cd-da0e-47b7-67cdae8ffb97@oracle.com>
On Mon, Oct 16, 2017 at 2:18 PM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> On 10/12/2017 03:53 PM, Boris Ostrovsky wrote:
>> On 10/12/2017 03:27 PM, Andrew Cooper wrote:
>>> On 12/10/17 20:11, Boris Ostrovsky wrote:
>>>> There is also another problem:
>>>>
>>>> [ 1.312425] general protection fault: 0000 [#1] SMP
>>>> [ 1.312901] Modules linked in:
>>>> [ 1.313389] CPU: 0 PID: 1 Comm: init Not tainted 4.14.0-rc4+ #6
>>>> [ 1.313878] task: ffff88003e2c0000 task.stack: ffffc9000038c000
>>>> [ 1.314360] RIP: 10000e030:entry_SYSCALL_64_fastpath+0x1/0xa5
>>>> [ 1.314854] RSP: e02b:ffffc9000038ff50 EFLAGS: 00010046
>>>> [ 1.315336] RAX: 000000000000000c RBX: 000055f550168040 RCX:
>>>> 00007fcfc959f59a
>>>> [ 1.315827] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
>>>> 0000000000000000
>>>> [ 1.316315] RBP: 000000000000000a R08: 000000000000037f R09:
>>>> 0000000000000064
>>>> [ 1.316805] R10: 000000001f89cbf5 R11: ffff88003e2c0000 R12:
>>>> 00007fcfc958ad60
>>>> [ 1.317300] R13: 0000000000000000 R14: 000055f550185954 R15:
>>>> 0000000000001000
>>>> [ 1.317801] FS: 0000000000000000(0000) GS:ffff88003f800000(0000)
>>>> knlGS:0000000000000000
>>>> [ 1.318267] CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [ 1.318750] CR2: 00007fcfc97ab218 CR3: 000000003c88e000 CR4:
>>>> 0000000000042660
>>>> [ 1.319235] Call Trace:
>>>> [ 1.319700] Code: 51 50 57 56 52 51 6a da 41 50 41 51 41 52 41 53 48
>>>> 83 ec 30 65 4c 8b 1c 25 c0 d2 00 00 41 f7 03 df 39 08 90 0f 85 a5 00 00
>>>> 00 50 <ff> 15 9c 95 d0 ff 58 48 3d 4c 01 00 00 77 0f 4c 89 d1 ff 14 c5
>>>> [ 1.321161] RIP: entry_SYSCALL_64_fastpath+0x1/0xa5 RSP: ffffc9000038ff50
>>>> [ 1.344255] ---[ end trace d7cb8cd6cd7c294c ]---
>>>> [ 1.345009] Kernel panic - not syncing: Attempted to kill init!
>>>> exitcode=0x0000000b
>>>>
>>>>
>>>> All code
>>>> ========
>>>> 0: 51 push %rcx
>>>> 1: 50 push %rax
>>>> 2: 57 push %rdi
>>>> 3: 56 push %rsi
>>>> 4: 52 push %rdx
>>>> 5: 51 push %rcx
>>>> 6: 6a da pushq $0xffffffffffffffda
>>>> 8: 41 50 push %r8
>>>> a: 41 51 push %r9
>>>> c: 41 52 push %r10
>>>> e: 41 53 push %r11
>>>> 10: 48 83 ec 30 sub $0x30,%rsp
>>>> 14: 65 4c 8b 1c 25 c0 d2 mov %gs:0xd2c0,%r11
>>>> 1b: 00 00
>>>> 1d: 41 f7 03 df 39 08 90 testl $0x900839df,(%r11)
>>>> 24: 0f 85 a5 00 00 00 jne 0xcf
>>>> 2a: 50 push %rax
>>>> 2b:* ff 15 9c 95 d0 ff callq *-0x2f6a64(%rip) #
>>>> 0xffffffffffd095cd <-- trapping instruction
>>>> 31: 58 pop %rax
>>>> 32: 48 3d 4c 01 00 00 cmp $0x14c,%rax
>>>> 38: 77 0f ja 0x49
>>>> 3a: 4c 89 d1 mov %r10,%rcx
>>>> 3d: ff .byte 0xff
>>>> 3e: 14 c5 adc $0xc5,%al
>>>>
>>>>
>>>> so the original 'cli' was replaced with the pv call but to me the offset
>>>> looks a bit off, no? Shouldn't it always be positive?
>>> callq takes a 32bit signed displacement, so jumping back by up to 2G is
>>> perfectly legitimate.
>> Yes, but
>>
>> ostr@workbase> nm vmlinux | grep entry_SYSCALL_64_fastpath
>> ffffffff817365dd t entry_SYSCALL_64_fastpath
>> ostr@workbase> nm vmlinux | grep " pv_irq_ops"
>> ffffffff81c2dbc0 D pv_irq_ops
>> ostr@workbase>
>>
>> so pv_irq_ops.irq_disable is about 5MB ahead of where we are now. (I
>> didn't mean that x86 instruction set doesn't allow negative
>> displacement, I was trying to say that pv_irq_ops always live further down)
>
> I believe the problem is this:
>
> #define PV_INDIRECT(addr) *addr(%rip)
>
> The displacement that the linker computes will be relative to the where
> this instruction is placed at the time of linking, which is in
> .pv_altinstructions (and not .text). So when we copy it into .text the
> displacement becomes bogus.
>
> Replacing the macro with
>
> #define PV_INDIRECT(addr) *addr // well, it's not so much
> indirect anymore
>
> makes things work. Or maybe it can be adjusted top be kept truly indirect.
That is still an indirect call, just using absolute addressing for the
pointer instead of RIP-relative. Alternatives has very limited
relocation capabilities. It will only handle a single call or jmp
replacement. Using absolute addressing is slightly less efficient
(takes one extra byte to encode, and needs a relocation for KASLR),
but it works just as well. You could also relocate the instruction
manually by adding the delta between the original and replacement code
to the displacement.
--
Brian Gerst
^ permalink raw reply
* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Josh Poimboeuf @ 2017-10-17 5:24 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Juergen Gross, Mike Galbraith, Ingo Molnar, Peter Zijlstra,
Andrew Cooper, Rusty Russell, virtualization, x86, linux-kernel,
Chris Wright, live-patching, Linus Torvalds, Borislav Petkov,
Andy Lutomirski, H. Peter Anvin, xen-devel, Thomas Gleixner,
Sasha Levin, Jiri Slaby, Alok Kataria
In-Reply-To: <b9fdc3ec-87cd-da0e-47b7-67cdae8ffb97@oracle.com>
On Mon, Oct 16, 2017 at 02:18:48PM -0400, Boris Ostrovsky wrote:
> On 10/12/2017 03:53 PM, Boris Ostrovsky wrote:
> > On 10/12/2017 03:27 PM, Andrew Cooper wrote:
> >> On 12/10/17 20:11, Boris Ostrovsky wrote:
> >>> There is also another problem:
> >>>
> >>> [ 1.312425] general protection fault: 0000 [#1] SMP
> >>> [ 1.312901] Modules linked in:
> >>> [ 1.313389] CPU: 0 PID: 1 Comm: init Not tainted 4.14.0-rc4+ #6
> >>> [ 1.313878] task: ffff88003e2c0000 task.stack: ffffc9000038c000
> >>> [ 1.314360] RIP: 10000e030:entry_SYSCALL_64_fastpath+0x1/0xa5
> >>> [ 1.314854] RSP: e02b:ffffc9000038ff50 EFLAGS: 00010046
> >>> [ 1.315336] RAX: 000000000000000c RBX: 000055f550168040 RCX:
> >>> 00007fcfc959f59a
> >>> [ 1.315827] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> >>> 0000000000000000
> >>> [ 1.316315] RBP: 000000000000000a R08: 000000000000037f R09:
> >>> 0000000000000064
> >>> [ 1.316805] R10: 000000001f89cbf5 R11: ffff88003e2c0000 R12:
> >>> 00007fcfc958ad60
> >>> [ 1.317300] R13: 0000000000000000 R14: 000055f550185954 R15:
> >>> 0000000000001000
> >>> [ 1.317801] FS: 0000000000000000(0000) GS:ffff88003f800000(0000)
> >>> knlGS:0000000000000000
> >>> [ 1.318267] CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>> [ 1.318750] CR2: 00007fcfc97ab218 CR3: 000000003c88e000 CR4:
> >>> 0000000000042660
> >>> [ 1.319235] Call Trace:
> >>> [ 1.319700] Code: 51 50 57 56 52 51 6a da 41 50 41 51 41 52 41 53 48
> >>> 83 ec 30 65 4c 8b 1c 25 c0 d2 00 00 41 f7 03 df 39 08 90 0f 85 a5 00 00
> >>> 00 50 <ff> 15 9c 95 d0 ff 58 48 3d 4c 01 00 00 77 0f 4c 89 d1 ff 14 c5
> >>> [ 1.321161] RIP: entry_SYSCALL_64_fastpath+0x1/0xa5 RSP: ffffc9000038ff50
> >>> [ 1.344255] ---[ end trace d7cb8cd6cd7c294c ]---
> >>> [ 1.345009] Kernel panic - not syncing: Attempted to kill init!
> >>> exitcode=0x0000000b
> >>>
> >>>
> >>> All code
> >>> ========
> >>> 0: 51 push %rcx
> >>> 1: 50 push %rax
> >>> 2: 57 push %rdi
> >>> 3: 56 push %rsi
> >>> 4: 52 push %rdx
> >>> 5: 51 push %rcx
> >>> 6: 6a da pushq $0xffffffffffffffda
> >>> 8: 41 50 push %r8
> >>> a: 41 51 push %r9
> >>> c: 41 52 push %r10
> >>> e: 41 53 push %r11
> >>> 10: 48 83 ec 30 sub $0x30,%rsp
> >>> 14: 65 4c 8b 1c 25 c0 d2 mov %gs:0xd2c0,%r11
> >>> 1b: 00 00
> >>> 1d: 41 f7 03 df 39 08 90 testl $0x900839df,(%r11)
> >>> 24: 0f 85 a5 00 00 00 jne 0xcf
> >>> 2a: 50 push %rax
> >>> 2b:* ff 15 9c 95 d0 ff callq *-0x2f6a64(%rip) #
> >>> 0xffffffffffd095cd <-- trapping instruction
> >>> 31: 58 pop %rax
> >>> 32: 48 3d 4c 01 00 00 cmp $0x14c,%rax
> >>> 38: 77 0f ja 0x49
> >>> 3a: 4c 89 d1 mov %r10,%rcx
> >>> 3d: ff .byte 0xff
> >>> 3e: 14 c5 adc $0xc5,%al
> >>>
> >>>
> >>> so the original 'cli' was replaced with the pv call but to me the offset
> >>> looks a bit off, no? Shouldn't it always be positive?
> >> callq takes a 32bit signed displacement, so jumping back by up to 2G is
> >> perfectly legitimate.
> > Yes, but
> >
> > ostr@workbase> nm vmlinux | grep entry_SYSCALL_64_fastpath
> > ffffffff817365dd t entry_SYSCALL_64_fastpath
> > ostr@workbase> nm vmlinux | grep " pv_irq_ops"
> > ffffffff81c2dbc0 D pv_irq_ops
> > ostr@workbase>
> >
> > so pv_irq_ops.irq_disable is about 5MB ahead of where we are now. (I
> > didn't mean that x86 instruction set doesn't allow negative
> > displacement, I was trying to say that pv_irq_ops always live further down)
>
> I believe the problem is this:
>
> #define PV_INDIRECT(addr) *addr(%rip)
>
> The displacement that the linker computes will be relative to the where
> this instruction is placed at the time of linking, which is in
> .pv_altinstructions (and not .text). So when we copy it into .text the
> displacement becomes bogus.
apply_alternatives() is supposed to adjust that displacement based on
the new IP, though it could be messing that up somehow. (See patch
10/13.)
--
Josh
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Michael S. Tsirkin @ 2017-10-17 3:44 UTC (permalink / raw)
To: Jason Wang
Cc: Willem de Bruijn, Network Development, David Miller,
virtualization
In-Reply-To: <ecf3dde0-d4e6-23d5-22a7-05ce45bdafc9@redhat.com>
On Tue, Oct 17, 2017 at 11:05:07AM +0800, Jason Wang wrote:
>
>
> On 2017年10月17日 06:34, Willem de Bruijn wrote:
> > On Mon, Oct 16, 2017 at 12:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Mon, Oct 16, 2017 at 12:04:57PM -0400, Willem de Bruijn wrote:
> > > > On Mon, Oct 16, 2017 at 11:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote:
> > > > > > > > +static int virtnet_reset(struct virtnet_info *vi)
> > > > > > > > +{
> > > > > > > > + struct virtio_device *dev = vi->vdev;
> > > > > > > > + int ret;
> > > > > > > > +
> > > > > > > > + virtio_config_disable(dev);
> > > > > > > > + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
> > > > > > > > + virtnet_freeze_down(dev, true);
> > > > > > > > + remove_vq_common(vi);
> > > > > > > > +
> > > > > > > > + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> > > > > > > > + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> > > > > > > > +
> > > > > > > > + ret = virtio_finalize_features(dev);
> > > > > > > > + if (ret)
> > > > > > > > + goto err;
> > > > > > > > +
> > > > > > > > + ret = virtnet_restore_up(dev);
> > > > > > > > + if (ret)
> > > > > > > > + goto err;
> > > > > > > > +
> > > > > > > > + ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
> > > > > > > > + if (ret)
> > > > > > > > + goto err;
> > > > > > > > +
> > > > > > > > + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> > > > > > > > + virtio_config_enable(dev);
> > > > > > > > + return 0;
> > > > > > > > +
> > > > > > > > +err:
> > > > > > > > + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
> > > > > > > > + return ret;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
> > > > > > > > {
> > > > > > > > struct scatterlist sg;
> > > > > > > I have a question here though. How do things like MAC address
> > > > > > > get restored?
> > > > > > >
> > > > > > > What about the rx mode?
> > > > > > >
> > > > > > > vlans?
> > > > > > The function as is releases and reinitializes only ring state.
> > > > > > Device configuration such as mac and vlan persist across
> > > > > > the reset.
> > > > > What gave you this impression? Take a look at e.g. this
> > > > > code in qemu:
> > > > >
> > > > > static void virtio_net_reset(VirtIODevice *vdev)
> > > > > {
> > > > > VirtIONet *n = VIRTIO_NET(vdev);
> > > > >
> > > > > /* Reset back to compatibility mode */
> > > > > n->promisc = 1;
> > > > > n->allmulti = 0;
> > > > > n->alluni = 0;
> > > > > n->nomulti = 0;
> > > > > n->nouni = 0;
> > > > > n->nobcast = 0;
> > > > > /* multiqueue is disabled by default */
> > > > > n->curr_queues = 1;
> > > > > timer_del(n->announce_timer);
> > > > > n->announce_counter = 0;
> > > > > n->status &= ~VIRTIO_NET_S_ANNOUNCE;
> > > > >
> > > > > /* Flush any MAC and VLAN filter table state */
> > > > > n->mac_table.in_use = 0;
> > > > > n->mac_table.first_multi = 0;
> > > > > n->mac_table.multi_overflow = 0;
> > > > > n->mac_table.uni_overflow = 0;
> > > > > memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
> > > > > memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
> > > > > qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> > > > > memset(n->vlans, 0, MAX_VLAN >> 3);
> > > > > }
> > > > >
> > > > > So device seems to lose all state, you have to re-program it.
> > > > Oh, indeed! The guest does not reset its state, so it might
> > > > be out of sync with the host after the operation. Was this not
> > > > an issue when previously resetting in the context of xdp?
> > > I suspect it was broken back then, too.
> > Okay. I guess that in principle this is all programmable through
> > virtnet_set_rx_mode, virtnet_vlan_rx_add_vid, etc. But it's a
> > lot more complex than just restoring virtnet_reset. Will need to
> > be careful about concurrency issues at the least. Similar to the
> > ones you point out below.
> >
>
> The problem has been pointed out during developing virtio-net XDP. But it
> may not be a big issue since vhost_net ignores all kinds of the filters now.
>
> Thanks
It might not keep doing that in the future though.
And virtio-net in userspace doesn't ignore the filters.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Jason Wang @ 2017-10-17 3:05 UTC (permalink / raw)
To: Willem de Bruijn, Michael S. Tsirkin
Cc: Network Development, Willem de Bruijn, David Miller,
virtualization
In-Reply-To: <CAF=yD-KkUH6=S6Q=Tjtd2Wo=-ebDUt1or-DqjoRV4dTaD8G0WA@mail.gmail.com>
On 2017年10月17日 06:34, Willem de Bruijn wrote:
> On Mon, Oct 16, 2017 at 12:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Mon, Oct 16, 2017 at 12:04:57PM -0400, Willem de Bruijn wrote:
>>> On Mon, Oct 16, 2017 at 11:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote:
>>>>>>> +static int virtnet_reset(struct virtnet_info *vi)
>>>>>>> +{
>>>>>>> + struct virtio_device *dev = vi->vdev;
>>>>>>> + int ret;
>>>>>>> +
>>>>>>> + virtio_config_disable(dev);
>>>>>>> + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
>>>>>>> + virtnet_freeze_down(dev, true);
>>>>>>> + remove_vq_common(vi);
>>>>>>> +
>>>>>>> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>>>>>>> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
>>>>>>> +
>>>>>>> + ret = virtio_finalize_features(dev);
>>>>>>> + if (ret)
>>>>>>> + goto err;
>>>>>>> +
>>>>>>> + ret = virtnet_restore_up(dev);
>>>>>>> + if (ret)
>>>>>>> + goto err;
>>>>>>> +
>>>>>>> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
>>>>>>> + if (ret)
>>>>>>> + goto err;
>>>>>>> +
>>>>>>> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>>>>>>> + virtio_config_enable(dev);
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> +err:
>>>>>>> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>>>>>>> + return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
>>>>>>> {
>>>>>>> struct scatterlist sg;
>>>>>> I have a question here though. How do things like MAC address
>>>>>> get restored?
>>>>>>
>>>>>> What about the rx mode?
>>>>>>
>>>>>> vlans?
>>>>> The function as is releases and reinitializes only ring state.
>>>>> Device configuration such as mac and vlan persist across
>>>>> the reset.
>>>> What gave you this impression? Take a look at e.g. this
>>>> code in qemu:
>>>>
>>>> static void virtio_net_reset(VirtIODevice *vdev)
>>>> {
>>>> VirtIONet *n = VIRTIO_NET(vdev);
>>>>
>>>> /* Reset back to compatibility mode */
>>>> n->promisc = 1;
>>>> n->allmulti = 0;
>>>> n->alluni = 0;
>>>> n->nomulti = 0;
>>>> n->nouni = 0;
>>>> n->nobcast = 0;
>>>> /* multiqueue is disabled by default */
>>>> n->curr_queues = 1;
>>>> timer_del(n->announce_timer);
>>>> n->announce_counter = 0;
>>>> n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>>>>
>>>> /* Flush any MAC and VLAN filter table state */
>>>> n->mac_table.in_use = 0;
>>>> n->mac_table.first_multi = 0;
>>>> n->mac_table.multi_overflow = 0;
>>>> n->mac_table.uni_overflow = 0;
>>>> memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
>>>> memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
>>>> qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
>>>> memset(n->vlans, 0, MAX_VLAN >> 3);
>>>> }
>>>>
>>>> So device seems to lose all state, you have to re-program it.
>>> Oh, indeed! The guest does not reset its state, so it might
>>> be out of sync with the host after the operation. Was this not
>>> an issue when previously resetting in the context of xdp?
>> I suspect it was broken back then, too.
> Okay. I guess that in principle this is all programmable through
> virtnet_set_rx_mode, virtnet_vlan_rx_add_vid, etc. But it's a
> lot more complex than just restoring virtnet_reset. Will need to
> be careful about concurrency issues at the least. Similar to the
> ones you point out below.
>
The problem has been pointed out during developing virtio-net XDP. But
it may not be a big issue since vhost_net ignores all kinds of the
filters now.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Willem de Bruijn @ 2017-10-16 22:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Willem de Bruijn, Network Development, David Miller,
virtualization
In-Reply-To: <20171016191806-mutt-send-email-mst@kernel.org>
On Mon, Oct 16, 2017 at 12:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Oct 16, 2017 at 12:04:57PM -0400, Willem de Bruijn wrote:
>> On Mon, Oct 16, 2017 at 11:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote:
>> >> >> +static int virtnet_reset(struct virtnet_info *vi)
>> >> >> +{
>> >> >> + struct virtio_device *dev = vi->vdev;
>> >> >> + int ret;
>> >> >> +
>> >> >> + virtio_config_disable(dev);
>> >> >> + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
>> >> >> + virtnet_freeze_down(dev, true);
>> >> >> + remove_vq_common(vi);
>> >> >> +
>> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
>> >> >> +
>> >> >> + ret = virtio_finalize_features(dev);
>> >> >> + if (ret)
>> >> >> + goto err;
>> >> >> +
>> >> >> + ret = virtnet_restore_up(dev);
>> >> >> + if (ret)
>> >> >> + goto err;
>> >> >> +
>> >> >> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
>> >> >> + if (ret)
>> >> >> + goto err;
>> >> >> +
>> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>> >> >> + virtio_config_enable(dev);
>> >> >> + return 0;
>> >> >> +
>> >> >> +err:
>> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>> >> >> + return ret;
>> >> >> +}
>> >> >> +
>> >> >> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
>> >> >> {
>> >> >> struct scatterlist sg;
>> >> >
>> >> > I have a question here though. How do things like MAC address
>> >> > get restored?
>> >> >
>> >> > What about the rx mode?
>> >> >
>> >> > vlans?
>> >>
>> >> The function as is releases and reinitializes only ring state.
>> >> Device configuration such as mac and vlan persist across
>> >> the reset.
>> >
>> > What gave you this impression? Take a look at e.g. this
>> > code in qemu:
>> >
>> > static void virtio_net_reset(VirtIODevice *vdev)
>> > {
>> > VirtIONet *n = VIRTIO_NET(vdev);
>> >
>> > /* Reset back to compatibility mode */
>> > n->promisc = 1;
>> > n->allmulti = 0;
>> > n->alluni = 0;
>> > n->nomulti = 0;
>> > n->nouni = 0;
>> > n->nobcast = 0;
>> > /* multiqueue is disabled by default */
>> > n->curr_queues = 1;
>> > timer_del(n->announce_timer);
>> > n->announce_counter = 0;
>> > n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>> >
>> > /* Flush any MAC and VLAN filter table state */
>> > n->mac_table.in_use = 0;
>> > n->mac_table.first_multi = 0;
>> > n->mac_table.multi_overflow = 0;
>> > n->mac_table.uni_overflow = 0;
>> > memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
>> > memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
>> > qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
>> > memset(n->vlans, 0, MAX_VLAN >> 3);
>> > }
>> >
>> > So device seems to lose all state, you have to re-program it.
>>
>> Oh, indeed! The guest does not reset its state, so it might
>> be out of sync with the host after the operation. Was this not
>> an issue when previously resetting in the context of xdp?
>
> I suspect it was broken back then, too.
Okay. I guess that in principle this is all programmable through
virtnet_set_rx_mode, virtnet_vlan_rx_add_vid, etc. But it's a
lot more complex than just restoring virtnet_reset. Will need to
be careful about concurrency issues at the least. Similar to the
ones you point out below.
>
>> >> > Also, it seems that LINK_ANNOUNCE requests will get ignored
>> >> > even if they got set before the reset, leading to downtime.
>> >>
>> >> Do you mean act on VIRTIO_NET_F_GUEST_ANNOUNCE
>> >> requests? That flag is tested and netdev_notify_peers
>> >> called before resetting virtio ring state.
>> >
>> > Yes but I wonder if there's a race where announce
>> > is set after it is read but before NEED_RESET is read.
>> >
>> > Re-reading status from the config before reset
>> > might be necessary.
>>
>> Thanks, I'll have a look. Perhaps a host should simply not
>> request a reset while it is waiting for an announce ack.
>
> It's one option though we can't make this change for existing hosts.
> We also have the reverse condition where announce is requested after
> NEED_RESET is set.
^ permalink raw reply
* Re: [Xen-devel] [PATCH 11/13] x86/paravirt: Add paravirt alternatives infrastructure
From: Boris Ostrovsky @ 2017-10-16 18:18 UTC (permalink / raw)
To: Andrew Cooper, Josh Poimboeuf
Cc: Juergen Gross, Mike Galbraith, Peter Zijlstra, Linus Torvalds,
Rusty Russell, virtualization, x86, linux-kernel, Chris Wright,
live-patching, Ingo Molnar, Borislav Petkov, Andy Lutomirski,
H. Peter Anvin, xen-devel, Thomas Gleixner, Sasha Levin,
Jiri Slaby, Alok Kataria
In-Reply-To: <a6c3be0b-35f4-8d23-4ea2-ead94a3fc69e@oracle.com>
On 10/12/2017 03:53 PM, Boris Ostrovsky wrote:
> On 10/12/2017 03:27 PM, Andrew Cooper wrote:
>> On 12/10/17 20:11, Boris Ostrovsky wrote:
>>> There is also another problem:
>>>
>>> [ 1.312425] general protection fault: 0000 [#1] SMP
>>> [ 1.312901] Modules linked in:
>>> [ 1.313389] CPU: 0 PID: 1 Comm: init Not tainted 4.14.0-rc4+ #6
>>> [ 1.313878] task: ffff88003e2c0000 task.stack: ffffc9000038c000
>>> [ 1.314360] RIP: 10000e030:entry_SYSCALL_64_fastpath+0x1/0xa5
>>> [ 1.314854] RSP: e02b:ffffc9000038ff50 EFLAGS: 00010046
>>> [ 1.315336] RAX: 000000000000000c RBX: 000055f550168040 RCX:
>>> 00007fcfc959f59a
>>> [ 1.315827] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
>>> 0000000000000000
>>> [ 1.316315] RBP: 000000000000000a R08: 000000000000037f R09:
>>> 0000000000000064
>>> [ 1.316805] R10: 000000001f89cbf5 R11: ffff88003e2c0000 R12:
>>> 00007fcfc958ad60
>>> [ 1.317300] R13: 0000000000000000 R14: 000055f550185954 R15:
>>> 0000000000001000
>>> [ 1.317801] FS: 0000000000000000(0000) GS:ffff88003f800000(0000)
>>> knlGS:0000000000000000
>>> [ 1.318267] CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [ 1.318750] CR2: 00007fcfc97ab218 CR3: 000000003c88e000 CR4:
>>> 0000000000042660
>>> [ 1.319235] Call Trace:
>>> [ 1.319700] Code: 51 50 57 56 52 51 6a da 41 50 41 51 41 52 41 53 48
>>> 83 ec 30 65 4c 8b 1c 25 c0 d2 00 00 41 f7 03 df 39 08 90 0f 85 a5 00 00
>>> 00 50 <ff> 15 9c 95 d0 ff 58 48 3d 4c 01 00 00 77 0f 4c 89 d1 ff 14 c5
>>> [ 1.321161] RIP: entry_SYSCALL_64_fastpath+0x1/0xa5 RSP: ffffc9000038ff50
>>> [ 1.344255] ---[ end trace d7cb8cd6cd7c294c ]---
>>> [ 1.345009] Kernel panic - not syncing: Attempted to kill init!
>>> exitcode=0x0000000b
>>>
>>>
>>> All code
>>> ========
>>> 0: 51 push %rcx
>>> 1: 50 push %rax
>>> 2: 57 push %rdi
>>> 3: 56 push %rsi
>>> 4: 52 push %rdx
>>> 5: 51 push %rcx
>>> 6: 6a da pushq $0xffffffffffffffda
>>> 8: 41 50 push %r8
>>> a: 41 51 push %r9
>>> c: 41 52 push %r10
>>> e: 41 53 push %r11
>>> 10: 48 83 ec 30 sub $0x30,%rsp
>>> 14: 65 4c 8b 1c 25 c0 d2 mov %gs:0xd2c0,%r11
>>> 1b: 00 00
>>> 1d: 41 f7 03 df 39 08 90 testl $0x900839df,(%r11)
>>> 24: 0f 85 a5 00 00 00 jne 0xcf
>>> 2a: 50 push %rax
>>> 2b:* ff 15 9c 95 d0 ff callq *-0x2f6a64(%rip) #
>>> 0xffffffffffd095cd <-- trapping instruction
>>> 31: 58 pop %rax
>>> 32: 48 3d 4c 01 00 00 cmp $0x14c,%rax
>>> 38: 77 0f ja 0x49
>>> 3a: 4c 89 d1 mov %r10,%rcx
>>> 3d: ff .byte 0xff
>>> 3e: 14 c5 adc $0xc5,%al
>>>
>>>
>>> so the original 'cli' was replaced with the pv call but to me the offset
>>> looks a bit off, no? Shouldn't it always be positive?
>> callq takes a 32bit signed displacement, so jumping back by up to 2G is
>> perfectly legitimate.
> Yes, but
>
> ostr@workbase> nm vmlinux | grep entry_SYSCALL_64_fastpath
> ffffffff817365dd t entry_SYSCALL_64_fastpath
> ostr@workbase> nm vmlinux | grep " pv_irq_ops"
> ffffffff81c2dbc0 D pv_irq_ops
> ostr@workbase>
>
> so pv_irq_ops.irq_disable is about 5MB ahead of where we are now. (I
> didn't mean that x86 instruction set doesn't allow negative
> displacement, I was trying to say that pv_irq_ops always live further down)
I believe the problem is this:
#define PV_INDIRECT(addr) *addr(%rip)
The displacement that the linker computes will be relative to the where
this instruction is placed at the time of linking, which is in
.pv_altinstructions (and not .text). So when we copy it into .text the
displacement becomes bogus.
Replacing the macro with
#define PV_INDIRECT(addr) *addr // well, it's not so much
indirect anymore
makes things work. Or maybe it can be adjusted top be kept truly indirect.
-boris
^ permalink raw reply
* CFP VEHITS 2018 - 4th Int.l Conf. on Vehicle Technology and Intelligent Transport Systems (Funchal, Madeira/Portugal)
From: vehits @ 2017-10-16 17:57 UTC (permalink / raw)
To: virtualization
SUBMISSION DEADLINE
4th International Conference on Vehicle Technology and Intelligent Transport Systems
Submission Deadline: October 30, 2017
http://www.vehits.org/
March 16 - 18, 2018
Funchal, Madeira, Portugal.
VEHITS is organized in 5 major tracks:
- Intelligent Vehicle Technologies
- Intelligent Transport Systems and Infrastructure
- Connected Vehicles
- Sustainable Transport
- Data Analtyics
In Cooperation with APVE, CVTA, ABVE, ITS Portugal.
With the presence of internationally distinguished keynote speakers:
Miguel A. Sotelo, Universidad de Alcalá, Spain
Karl-Heinz Krempels, RWTH Aachen University, Germany
A short list of presented papers will be selected so that revised and extended versions of these papers will be published by Springer.
All papers presented at the congress venue will also be available at the SCITEPRESS Digital Library (http://www.scitepress.org/DigitalLibrary/).
Should you have any question please don’t hesitate contacting me.
Kind regards,
VEHITS Secretariat
Address: Av. D. Manuel I, 27A, 2º esq.
2910-595 Setubal, Portugal
Tel: +351 265 520 185
Fax: +351 265 520 186
Web: http://www.vehits.org/
e-mail: vehits.secretariat@insticc.org
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* CFP SMARTGREENS 2018 - 7th Int.l Conf. on Smart Cities and Green ICT Systems (Funchal, Madeira/Portugal)
From: smartgreens @ 2017-10-16 17:57 UTC (permalink / raw)
To: virtualization
SUBMISSION DEADLINE
7th International Conference on Smart Cities and Green ICT Systems
Submission Deadline: October 30, 2017
http://www.smartgreens.org/
March 16 - 18, 2018
Funchal, Madeira, Portugal.
SMARTGREENS is organized in 4 major tracks:
- Energy-Aware Systems and Technologies
- Sustainable Computing and Systems
- Smart Cities and Smart Buildings
- Demos and Use-Cases
In Cooperation with IEEE Technical Area in Green Computing, Siemens, EDSO, APEA, OSGP Alliance, Smart Cities Council.
With the presence of internationally distinguished keynote speakers:
Miguel A. Sotelo, Universidad de Alcalá, Spain
A short list of presented papers will be selected so that revised and extended versions of these papers will be published by Springer.
All papers presented at the congress venue will also be available at the SCITEPRESS Digital Library (http://www.scitepress.org/DigitalLibrary/).
Should you have any question please don’t hesitate contacting me.
Kind regards,
SMARTGREENS Secretariat
Address: Av. D. Manuel I, 27A, 2º esq.
2910-595 Setubal, Portugal
Tel: +351 265 520 185
Fax: +351 265 520 186
Web: http://www.smartgreens.org/
e-mail: smartgreens.secretariat@insticc.org
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* CFP IoTBDS 2018 - 3rd Int.l Conf. on Internet of Things, Big Data and Security (Funchal, Madeira/Portugal)
From: iotbds @ 2017-10-16 17:56 UTC (permalink / raw)
To: virtualization
SUBMISSION DEADLINE
3rd International Conference on Internet of Things, Big Data and Security
Submission Deadline: October 30, 2017
http://iotbds.org/
March 19 - 21, 2018
Funchal, Madeira, Portugal.
IoTBDS is organized in 7 major tracks:
- Big Data Research
- Emerging Services and Analytics
- Internet of Things (IoT) Fundamentals
- Internet of Things (IoT) Applications
- Big Data for Multi-discipline Services
- Security, Privacy and Trust
- IoT Technologies
In Cooperation with EuroCloud, TICE.
With the presence of internationally distinguished keynote speakers:
Tobias Hoellwarth, EuroCloud Europe, Austria
Péter Kacsuk, MTA SZTAKI, Hungary
A short list of presented papers will be selected so that revised and extended versions of these papers will be published by Springer.
All papers presented at the congress venue will also be available at the SCITEPRESS Digital Library (http://www.scitepress.org/DigitalLibrary/).
Should you have any question please don’t hesitate contacting me.
Kind regards,
IoTBDS Secretariat
Address: Av. D. Manuel I, 27A, 2º esq.
2910-595 Setubal, Portugal
Tel: +351 265 520 184
Fax: +351 265 520 186
Web: http://iotbds.org/
e-mail: iotbds.secretariat@insticc.org
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* CFP CLOSER 2018 - 8th Int.l Conf. on Cloud Computing and Services Science (Funchal, Madeira/Portugal)
From: closer @ 2017-10-16 17:56 UTC (permalink / raw)
To: virtualization
SUBMISSION DEADLINE
8th International Conference on Cloud Computing and Services Science
Submission Deadline: October 30, 2017
http://closer.scitevents.org/
March 19 - 21, 2018
Funchal, Madeira, Portugal.
CLOSER is organized in 9 major tracks:
- Services Science
- Data as a Service
- Cloud Operations
- Edge Cloud and Fog Computing
- Service Modelling and Analytics
- Mobile Cloud Computing
- Cloud Computing Fundamentals
- Cloud Computing Platforms and Applications
- Cloud Computing Enabling Technology
In Cooperation with EuroCloud.
With the presence of internationally distinguished keynote speakers:
Mike Papazoglou, Tilburg University, Netherlands
Tobias Hoellwarth, EuroCloud Europe, Austria
Péter Kacsuk, MTA SZTAKI, Hungary
Lee Gillam, University of Surrey , United Kingdom
A short list of presented papers will be selected so that revised and extended versions of these papers will be published by Springer.
All papers presented at the congress venue will also be available at the SCITEPRESS Digital Library (http://www.scitepress.org/DigitalLibrary/).
Should you have any question please don’t hesitate contacting me.
Kind regards,
CLOSER Secretariat
Address: Av. D. Manuel I, 27A, 2º esq.
2910-595 Setubal, Portugal
Tel: +351 265 520 184
Fax: +351 265 520 186
Web: http://closer.scitevents.org/
e-mail: closer.secretariat@insticc.org
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
From: Michael S. Tsirkin @ 2017-10-16 17:01 UTC (permalink / raw)
To: Tetsuo Handa
Cc: virtualization, linux-mm, Raushaniya Maksudova, Denis V. Lunev,
mhocko
In-Reply-To: <201710161958.IAE65151.HFOLMQSFOVFJtO@I-love.SAKURA.ne.jp>
On Mon, Oct 16, 2017 at 07:58:29PM +0900, Tetsuo Handa wrote:
> Tetsuo Handa wrote:
> > Michael S. Tsirkin wrote:
> > > > >
> > > > > The proper fix isn't that hard - just avoid allocations under lock.
> > > > >
> > > > > Patch posted, pls take a look.
> > > >
> > > > Your patch allocates pages in order to inflate the balloon, but
> > > > your patch will allow leak_balloon() to deflate the balloon.
> > > > How deflating the balloon (i.e. calling leak_balloon()) makes sense
> > > > when allocating pages for inflating the balloon (i.e. calling
> > > > fill_balloon()) ?
> > >
> > > The idea is that fill_balloon is allocating memory with __GFP_NORETRY
> > > so it will avoid disruptive actions like the OOM killer.
> > > Under pressure it will normally fail and retry in half a second or so.
> > >
> > > Calling leak_balloon in that situation could benefit the system as a whole.
> > >
> > > I might be misunderstanding the meaning of the relevant GFP flags,
> > > pls correct me if I'm wrong.
> >
> > Would you answer to below question by "yes"/"no" ?
> >
> > If leak_balloon() is called via out_of_memory(), leak_balloon()
> > will decrease "struct virtio_balloon"->num_pages.
> > But, is "struct virtio_balloon_config"->num_pages updated when
> > leak_balloon() is called via out_of_memory() ?
> >
> > Below explanation assumes that your answer is "no".
> >
> > I consider that fill_balloon() is using __GFP_NORETRY is a bug.
>
> Below are my test results using 4.14-rc5.
>
> http://I-love.SAKURA.ne.jp/tmp/20171016-default.log.xz
> http://I-love.SAKURA.ne.jp/tmp/20171016-deflate.log.xz
>
> 20171016-default.log.xz is without VIRTIO_BALLOON_F_DEFLATE_ON_OOM and
> 20171016-deflate.log.xz is with VIRTIO_BALLOON_F_DEFLATE_ON_OOM. (I used
> inverting virtio_has_feature(VIRTIO_BALLOON_F_DEFLATE_ON_OOM) test
> because the QEMU I'm using does not support deflate-on-oom option.)
>
> > Consider an extreme situation that guest1 is started with 8192MB
> > memory and then guest1's memory is reduced to 128MB by
> >
> > virsh qemu-monitor-command --domain guest1 --hmp 'balloon 128'
> >
> > when VIRTIO_BALLOON_F_DEFLATE_ON_OOM was not negotiated.
> > Of course, 128MB would be too small to operate guest1 properly.
> > Since update_balloon_size_func() continues calling fill_balloon()
> > until guest1's memory is reduced to 128MB, you will see flooding of
> > "puff" messages (and guest1 is practically unusable because all CPU
> > resource will be wasted for unsuccessful memory reclaim attempts)
> > unless the OOM killer is invoked.
>
> 20171016-default.log.xz continued printing "puff" messages until kernel
> panic caused by somebody else killing all OOM killable processes via
> GFP_KERNEL allocation requests. Although fill_balloon() was printing
> a lot of noises due to __GFP_NORETRY, the system made forward progress
> in the form of kernel panic triggered by no more OOM killable processes.
>
> ----------
> [ 88.270838] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 88.503496] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 88.730058] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 88.937971] Out of memory: Kill process 669 (dhclient) score 54 or sacrifice child
> [ 89.007322] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 89.234874] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 89.439735] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 89.496389] Out of memory: Kill process 853 (tuned) score 47 or sacrifice child
> [ 89.663808] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 89.883812] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 90.104417] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 90.251293] Out of memory: Kill process 568 (polkitd) score 36 or sacrifice child
> [ 90.326131] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 90.821722] Out of memory: Kill process 601 (NetworkManager) score 23 or sacrifice child
> [ 91.293848] Out of memory: Kill process 585 (rsyslogd) score 13 or sacrifice child
> [ 91.799413] Out of memory: Kill process 415 (systemd-journal) score 13 or sacrifice child
> [ 91.861974] Out of memory: Kill process 987 (qmgr) score 6 or sacrifice child
> [ 91.925297] Out of memory: Kill process 985 (master) score 6 or sacrifice child
> [ 92.254082] Out of memory: Kill process 985 (master) score 6 or sacrifice child
> [ 92.464029] Out of memory: Kill process 881 (login) score 4 or sacrifice child
> [ 92.467859] Out of memory: Kill process 881 (login) score 4 or sacrifice child
> [ 93.490928] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 93.713220] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 93.932145] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 94.147652] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 94.363826] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 94.606404] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 94.833539] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 95.053230] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 95.267805] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 95.483789] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 98.290124] Out of memory: Kill process 595 (crond) score 3 or sacrifice child
> [ 98.643588] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 98.864487] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 99.084685] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 99.299766] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 99.515745] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 99.758334] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 99.985175] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 100.204474] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 100.419757] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 100.635681] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 101.263840] Out of memory: Kill process 587 (systemd-logind) score 2 or sacrifice child
> [ 101.319432] Out of memory: Kill process 586 (irqbalance) score 1 or sacrifice child
> [ 101.386546] Out of memory: Kill process 569 (dbus-daemon) score 0 or sacrifice child
> [ 103.805754] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 104.033073] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 104.253104] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 104.467810] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 104.683792] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 104.926409] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 105.153490] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 105.372610] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 105.587751] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 105.803719] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 108.958882] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 109.185254] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 109.404393] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 109.621584] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 109.697419] Kernel panic - not syncing: Out of memory and no killable processes...
> [ 109.723710] ---[ end Kernel panic - not syncing: Out of memory and no killable processes...
> ----------
>
> >
> > What this patch is trying to handle is a situation when
> > VIRTIO_BALLOON_F_DEFLATE_ON_OOM was negotiated. Once
> > update_balloon_size_func() started calling fill_balloon(),
> > update_balloon_size_func() will continue calling fill_balloon()
> > until guest1's memory is reduced to 128MB, won't it?
> >
> > Since fill_balloon() uses __GFP_IO | __GFP_FS, fill_balloon() can
> > indirectly trigger out_of_memory() despite __GFP_NORETRY is specified.
> >
> > When update_balloon_size_func() is running for calling fill_balloon(),
> > calling leak_balloon() will increase number of pages to fill which
> > fill_balloon() is supposed to fill. Leaking some pages from leak_balloon()
> > via blocking_notifier_call_chain() callback could avoid invocation of the
> > OOM killer for that specific moment, but it bounces back to us later because
> > number of pages to allocate later (note that update_balloon_size_func() is
> > running for calling fill_balloon()) is increased by leak_balloon().
>
> 20171016-deflate.log.xz continued printing "puff" messages without any OOM
> killer messages, for fill_balloon() always inflates faster than leak_balloon()
> deflates.
>
> Since the OOM killer cannot be invoked unless leak_balloon() completely
> deflates faster than fill_balloon() inflates, the guest remained unusable
> (e.g. unable to login via ssh) other than printing "puff" messages.
> This result was worse than 20171016-default.log.xz , for the system was
> not able to make any forward progress (i.e. complete OOM lockup).
>
> ----------
> [ 19.866938] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 20.089350] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 20.430965] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 20.652641] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 20.894680] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 21.122063] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 21.340872] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 21.556128] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 21.772473] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 22.014960] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 24.972961] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 25.201565] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 25.420875] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 25.636081] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 25.851670] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 26.095842] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 26.322284] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 26.851691] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> (...snipped...)
> [ 211.748567] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 211.963910] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 215.157737] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 215.385239] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 215.604426] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 215.819807] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 216.036491] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 216.278718] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 216.505410] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 216.724334] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 216.940318] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 217.155560] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 220.312187] virtio_balloon virtio3: Out of puff! Can't get 1 pages
> [ 220.342980] sysrq: SysRq : Resetting
> ----------
>
> >
> > Thus, I don't think that avoid invoking the OOM killer by calling leak_balloon()
> > makes sense when update_balloon_size_func() is running for calling fill_balloon().
> > And this patch tries to detect it by replacing mutex_lock() with mutex_trylock().
> >
> > > Well the point of this flag is that when it's acked,
> > > host knows that it's safe to inflate the balloon
> > > to a large portion of guest memory and this won't
> > > cause an OOM situation.
> >
> > Assuming that your answer to the question is "no", I don't think it is
> > safe to inflate the balloon to a large portion of guest memory, for once
> > update_balloon_size_func() started calling fill_balloon(),
> > update_balloon_size_func() can not stop calling fill_balloon() even when
> > blocking_notifier_call_chain() callback called leak_balloon() because
> > "struct virtio_balloon_config"->num_pages will not be updated when
> > blocking_notifier_call_chain() callback called leak_balloon().
>
> As I demonstrated above, VIRTIO_BALLOON_F_DEFLATE_ON_OOM can lead to complete
> OOM lockup because out_of_memory() => fill_balloon() => out_of_memory() =>
> fill_balloon() sequence can effectively disable the OOM killer when the host
> assumed that it's safe to inflate the balloon to a large portion of guest
> memory and this won't cause an OOM situation.
I agree. But I wonder how did the contributors use it in their systems.
> > > I think the assumption is that it fill back up eventually
> > > when guest does have some free memory.
>
> So, my question again because such assumption is broken.
> What is the expected behavior after deflating while inflating?
> How should "struct virtio_balloon_config"->num_pages be interpreted?
>
> struct virtio_balloon_config {
> /* Number of pages host wants Guest to give up. */
> __u32 num_pages;
> /* Number of pages we've actually got in balloon. */
> __u32 actual;
> };
>
> If leak_balloon() from out_of_memory() should be stronger than
> fill_balloon() from update_balloon_size_func(), we need to make
> sure that update_balloon_size_func() stops calling fill_balloon()
> when leak_balloon() was called from out_of_memory().
I think that's the case. Question is, when can we inflate again?
> If fill_balloon() from update_balloon_size_func() should be
> stronger than leak_balloon() from out_of_memory(), we need to make
> sure that leak_balloon() from out_of_memory() is ignored when
> fill_balloon() from update_balloon_size_func() is running.
>
> Apart from vb->balloon_lock deadlock avoidance, we need to define
> the expected behavior.
--
MST
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Michael S. Tsirkin @ 2017-10-16 16:38 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Willem de Bruijn, Network Development, David Miller,
virtualization
In-Reply-To: <CAF=yD-LW7f30V8Ci1SQUPZ-WBihqr93cWQP2QBFg2oQtytvAPg@mail.gmail.com>
On Mon, Oct 16, 2017 at 12:04:57PM -0400, Willem de Bruijn wrote:
> On Mon, Oct 16, 2017 at 11:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote:
> >> >> +static int virtnet_reset(struct virtnet_info *vi)
> >> >> +{
> >> >> + struct virtio_device *dev = vi->vdev;
> >> >> + int ret;
> >> >> +
> >> >> + virtio_config_disable(dev);
> >> >> + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
> >> >> + virtnet_freeze_down(dev, true);
> >> >> + remove_vq_common(vi);
> >> >> +
> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> >> >> +
> >> >> + ret = virtio_finalize_features(dev);
> >> >> + if (ret)
> >> >> + goto err;
> >> >> +
> >> >> + ret = virtnet_restore_up(dev);
> >> >> + if (ret)
> >> >> + goto err;
> >> >> +
> >> >> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
> >> >> + if (ret)
> >> >> + goto err;
> >> >> +
> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> >> >> + virtio_config_enable(dev);
> >> >> + return 0;
> >> >> +
> >> >> +err:
> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
> >> >> + return ret;
> >> >> +}
> >> >> +
> >> >> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
> >> >> {
> >> >> struct scatterlist sg;
> >> >
> >> > I have a question here though. How do things like MAC address
> >> > get restored?
> >> >
> >> > What about the rx mode?
> >> >
> >> > vlans?
> >>
> >> The function as is releases and reinitializes only ring state.
> >> Device configuration such as mac and vlan persist across
> >> the reset.
> >
> > What gave you this impression? Take a look at e.g. this
> > code in qemu:
> >
> > static void virtio_net_reset(VirtIODevice *vdev)
> > {
> > VirtIONet *n = VIRTIO_NET(vdev);
> >
> > /* Reset back to compatibility mode */
> > n->promisc = 1;
> > n->allmulti = 0;
> > n->alluni = 0;
> > n->nomulti = 0;
> > n->nouni = 0;
> > n->nobcast = 0;
> > /* multiqueue is disabled by default */
> > n->curr_queues = 1;
> > timer_del(n->announce_timer);
> > n->announce_counter = 0;
> > n->status &= ~VIRTIO_NET_S_ANNOUNCE;
> >
> > /* Flush any MAC and VLAN filter table state */
> > n->mac_table.in_use = 0;
> > n->mac_table.first_multi = 0;
> > n->mac_table.multi_overflow = 0;
> > n->mac_table.uni_overflow = 0;
> > memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
> > memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
> > qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> > memset(n->vlans, 0, MAX_VLAN >> 3);
> > }
> >
> > So device seems to lose all state, you have to re-program it.
>
> Oh, indeed! The guest does not reset its state, so it might
> be out of sync with the host after the operation. Was this not
> an issue when previously resetting in the context of xdp?
I suspect it was broken back then, too.
> >> > Also, it seems that LINK_ANNOUNCE requests will get ignored
> >> > even if they got set before the reset, leading to downtime.
> >>
> >> Do you mean act on VIRTIO_NET_F_GUEST_ANNOUNCE
> >> requests? That flag is tested and netdev_notify_peers
> >> called before resetting virtio ring state.
> >
> > Yes but I wonder if there's a race where announce
> > is set after it is read but before NEED_RESET is read.
> >
> > Re-reading status from the config before reset
> > might be necessary.
>
> Thanks, I'll have a look. Perhaps a host should simply not
> request a reset while it is waiting for an announce ack.
It's one option though we can't make this change for existing hosts.
We also have the reverse condition where announce is requested after
NEED_RESET is set.
--
MST
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Willem de Bruijn @ 2017-10-16 16:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Willem de Bruijn, Network Development, David Miller,
virtualization
In-Reply-To: <20171016180442-mutt-send-email-mst@kernel.org>
On Mon, Oct 16, 2017 at 11:31 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote:
>> >> +static int virtnet_reset(struct virtnet_info *vi)
>> >> +{
>> >> + struct virtio_device *dev = vi->vdev;
>> >> + int ret;
>> >> +
>> >> + virtio_config_disable(dev);
>> >> + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
>> >> + virtnet_freeze_down(dev, true);
>> >> + remove_vq_common(vi);
>> >> +
>> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
>> >> +
>> >> + ret = virtio_finalize_features(dev);
>> >> + if (ret)
>> >> + goto err;
>> >> +
>> >> + ret = virtnet_restore_up(dev);
>> >> + if (ret)
>> >> + goto err;
>> >> +
>> >> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
>> >> + if (ret)
>> >> + goto err;
>> >> +
>> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>> >> + virtio_config_enable(dev);
>> >> + return 0;
>> >> +
>> >> +err:
>> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>> >> + return ret;
>> >> +}
>> >> +
>> >> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
>> >> {
>> >> struct scatterlist sg;
>> >
>> > I have a question here though. How do things like MAC address
>> > get restored?
>> >
>> > What about the rx mode?
>> >
>> > vlans?
>>
>> The function as is releases and reinitializes only ring state.
>> Device configuration such as mac and vlan persist across
>> the reset.
>
> What gave you this impression? Take a look at e.g. this
> code in qemu:
>
> static void virtio_net_reset(VirtIODevice *vdev)
> {
> VirtIONet *n = VIRTIO_NET(vdev);
>
> /* Reset back to compatibility mode */
> n->promisc = 1;
> n->allmulti = 0;
> n->alluni = 0;
> n->nomulti = 0;
> n->nouni = 0;
> n->nobcast = 0;
> /* multiqueue is disabled by default */
> n->curr_queues = 1;
> timer_del(n->announce_timer);
> n->announce_counter = 0;
> n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>
> /* Flush any MAC and VLAN filter table state */
> n->mac_table.in_use = 0;
> n->mac_table.first_multi = 0;
> n->mac_table.multi_overflow = 0;
> n->mac_table.uni_overflow = 0;
> memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
> memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
> qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> memset(n->vlans, 0, MAX_VLAN >> 3);
> }
>
> So device seems to lose all state, you have to re-program it.
Oh, indeed! The guest does not reset its state, so it might
be out of sync with the host after the operation. Was this not
an issue when previously resetting in the context of xdp?
>> > Also, it seems that LINK_ANNOUNCE requests will get ignored
>> > even if they got set before the reset, leading to downtime.
>>
>> Do you mean act on VIRTIO_NET_F_GUEST_ANNOUNCE
>> requests? That flag is tested and netdev_notify_peers
>> called before resetting virtio ring state.
>
> Yes but I wonder if there's a race where announce
> is set after it is read but before NEED_RESET is read.
>
> Re-reading status from the config before reset
> might be necessary.
Thanks, I'll have a look. Perhaps a host should simply not
request a reset while it is waiting for an announce ack.
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Michael S. Tsirkin @ 2017-10-16 15:31 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Willem de Bruijn, Network Development, David Miller,
virtualization
In-Reply-To: <CAF=yD-LdGiKuf=k2FDgFWbhvExVX8t+VVgTQetKTegjMeQ598g@mail.gmail.com>
On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote:
> >> +static int virtnet_reset(struct virtnet_info *vi)
> >> +{
> >> + struct virtio_device *dev = vi->vdev;
> >> + int ret;
> >> +
> >> + virtio_config_disable(dev);
> >> + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
> >> + virtnet_freeze_down(dev, true);
> >> + remove_vq_common(vi);
> >> +
> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> >> +
> >> + ret = virtio_finalize_features(dev);
> >> + if (ret)
> >> + goto err;
> >> +
> >> + ret = virtnet_restore_up(dev);
> >> + if (ret)
> >> + goto err;
> >> +
> >> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
> >> + if (ret)
> >> + goto err;
> >> +
> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> >> + virtio_config_enable(dev);
> >> + return 0;
> >> +
> >> +err:
> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
> >> + return ret;
> >> +}
> >> +
> >> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
> >> {
> >> struct scatterlist sg;
> >
> > I have a question here though. How do things like MAC address
> > get restored?
> >
> > What about the rx mode?
> >
> > vlans?
>
> The function as is releases and reinitializes only ring state.
> Device configuration such as mac and vlan persist across
> the reset.
What gave you this impression? Take a look at e.g. this
code in qemu:
static void virtio_net_reset(VirtIODevice *vdev)
{
VirtIONet *n = VIRTIO_NET(vdev);
/* Reset back to compatibility mode */
n->promisc = 1;
n->allmulti = 0;
n->alluni = 0;
n->nomulti = 0;
n->nouni = 0;
n->nobcast = 0;
/* multiqueue is disabled by default */
n->curr_queues = 1;
timer_del(n->announce_timer);
n->announce_counter = 0;
n->status &= ~VIRTIO_NET_S_ANNOUNCE;
/* Flush any MAC and VLAN filter table state */
n->mac_table.in_use = 0;
n->mac_table.first_multi = 0;
n->mac_table.multi_overflow = 0;
n->mac_table.uni_overflow = 0;
memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
memset(n->vlans, 0, MAX_VLAN >> 3);
}
So device seems to lose all state, you have to re-program it.
> > Also, it seems that LINK_ANNOUNCE requests will get ignored
> > even if they got set before the reset, leading to downtime.
>
> Do you mean act on VIRTIO_NET_F_GUEST_ANNOUNCE
> requests? That flag is tested and netdev_notify_peers
> called before resetting virtio ring state.
Yes but I wonder if there's a race where announce
is set after it is read but before NEED_RESET is read.
Re-reading status from the config before reset
might be necessary.
--
MST
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Willem de Bruijn @ 2017-10-16 15:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Willem de Bruijn, Network Development, David Miller,
virtualization
In-Reply-To: <20171015034018-mutt-send-email-mst@kernel.org>
>> +static int virtnet_reset(struct virtnet_info *vi)
>> +{
>> + struct virtio_device *dev = vi->vdev;
>> + int ret;
>> +
>> + virtio_config_disable(dev);
>> + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
>> + virtnet_freeze_down(dev, true);
>> + remove_vq_common(vi);
>> +
>> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
>> +
>> + ret = virtio_finalize_features(dev);
>> + if (ret)
>> + goto err;
>> +
>> + ret = virtnet_restore_up(dev);
>> + if (ret)
>> + goto err;
>> +
>> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
>> + if (ret)
>> + goto err;
>> +
>> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>> + virtio_config_enable(dev);
>> + return 0;
>> +
>> +err:
>> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>> + return ret;
>> +}
>> +
>> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
>> {
>> struct scatterlist sg;
>
> I have a question here though. How do things like MAC address
> get restored?
>
> What about the rx mode?
>
> vlans?
The function as is releases and reinitializes only ring state.
Device configuration such as mac and vlan persist across
the reset.
> Also, it seems that LINK_ANNOUNCE requests will get ignored
> even if they got set before the reset, leading to downtime.
Do you mean act on VIRTIO_NET_F_GUEST_ANNOUNCE
requests? That flag is tested and netdev_notify_peers
called before resetting virtio ring state.
^ permalink raw reply
* Re: [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
From: Tetsuo Handa @ 2017-10-16 10:58 UTC (permalink / raw)
To: mst; +Cc: linux-mm, virtualization, mhocko
In-Reply-To: <201710151438.FAD86443.tOOFHVOSFQJLMF@I-love.SAKURA.ne.jp>
Tetsuo Handa wrote:
> Michael S. Tsirkin wrote:
> > > >
> > > > The proper fix isn't that hard - just avoid allocations under lock.
> > > >
> > > > Patch posted, pls take a look.
> > >
> > > Your patch allocates pages in order to inflate the balloon, but
> > > your patch will allow leak_balloon() to deflate the balloon.
> > > How deflating the balloon (i.e. calling leak_balloon()) makes sense
> > > when allocating pages for inflating the balloon (i.e. calling
> > > fill_balloon()) ?
> >
> > The idea is that fill_balloon is allocating memory with __GFP_NORETRY
> > so it will avoid disruptive actions like the OOM killer.
> > Under pressure it will normally fail and retry in half a second or so.
> >
> > Calling leak_balloon in that situation could benefit the system as a whole.
> >
> > I might be misunderstanding the meaning of the relevant GFP flags,
> > pls correct me if I'm wrong.
>
> Would you answer to below question by "yes"/"no" ?
>
> If leak_balloon() is called via out_of_memory(), leak_balloon()
> will decrease "struct virtio_balloon"->num_pages.
> But, is "struct virtio_balloon_config"->num_pages updated when
> leak_balloon() is called via out_of_memory() ?
>
> Below explanation assumes that your answer is "no".
>
> I consider that fill_balloon() is using __GFP_NORETRY is a bug.
Below are my test results using 4.14-rc5.
http://I-love.SAKURA.ne.jp/tmp/20171016-default.log.xz
http://I-love.SAKURA.ne.jp/tmp/20171016-deflate.log.xz
20171016-default.log.xz is without VIRTIO_BALLOON_F_DEFLATE_ON_OOM and
20171016-deflate.log.xz is with VIRTIO_BALLOON_F_DEFLATE_ON_OOM. (I used
inverting virtio_has_feature(VIRTIO_BALLOON_F_DEFLATE_ON_OOM) test
because the QEMU I'm using does not support deflate-on-oom option.)
> Consider an extreme situation that guest1 is started with 8192MB
> memory and then guest1's memory is reduced to 128MB by
>
> virsh qemu-monitor-command --domain guest1 --hmp 'balloon 128'
>
> when VIRTIO_BALLOON_F_DEFLATE_ON_OOM was not negotiated.
> Of course, 128MB would be too small to operate guest1 properly.
> Since update_balloon_size_func() continues calling fill_balloon()
> until guest1's memory is reduced to 128MB, you will see flooding of
> "puff" messages (and guest1 is practically unusable because all CPU
> resource will be wasted for unsuccessful memory reclaim attempts)
> unless the OOM killer is invoked.
20171016-default.log.xz continued printing "puff" messages until kernel
panic caused by somebody else killing all OOM killable processes via
GFP_KERNEL allocation requests. Although fill_balloon() was printing
a lot of noises due to __GFP_NORETRY, the system made forward progress
in the form of kernel panic triggered by no more OOM killable processes.
----------
[ 88.270838] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 88.503496] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 88.730058] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 88.937971] Out of memory: Kill process 669 (dhclient) score 54 or sacrifice child
[ 89.007322] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 89.234874] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 89.439735] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 89.496389] Out of memory: Kill process 853 (tuned) score 47 or sacrifice child
[ 89.663808] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 89.883812] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 90.104417] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 90.251293] Out of memory: Kill process 568 (polkitd) score 36 or sacrifice child
[ 90.326131] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 90.821722] Out of memory: Kill process 601 (NetworkManager) score 23 or sacrifice child
[ 91.293848] Out of memory: Kill process 585 (rsyslogd) score 13 or sacrifice child
[ 91.799413] Out of memory: Kill process 415 (systemd-journal) score 13 or sacrifice child
[ 91.861974] Out of memory: Kill process 987 (qmgr) score 6 or sacrifice child
[ 91.925297] Out of memory: Kill process 985 (master) score 6 or sacrifice child
[ 92.254082] Out of memory: Kill process 985 (master) score 6 or sacrifice child
[ 92.464029] Out of memory: Kill process 881 (login) score 4 or sacrifice child
[ 92.467859] Out of memory: Kill process 881 (login) score 4 or sacrifice child
[ 93.490928] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 93.713220] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 93.932145] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 94.147652] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 94.363826] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 94.606404] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 94.833539] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 95.053230] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 95.267805] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 95.483789] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 98.290124] Out of memory: Kill process 595 (crond) score 3 or sacrifice child
[ 98.643588] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 98.864487] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 99.084685] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 99.299766] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 99.515745] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 99.758334] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 99.985175] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 100.204474] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 100.419757] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 100.635681] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 101.263840] Out of memory: Kill process 587 (systemd-logind) score 2 or sacrifice child
[ 101.319432] Out of memory: Kill process 586 (irqbalance) score 1 or sacrifice child
[ 101.386546] Out of memory: Kill process 569 (dbus-daemon) score 0 or sacrifice child
[ 103.805754] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 104.033073] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 104.253104] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 104.467810] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 104.683792] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 104.926409] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 105.153490] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 105.372610] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 105.587751] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 105.803719] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 108.958882] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 109.185254] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 109.404393] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 109.621584] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 109.697419] Kernel panic - not syncing: Out of memory and no killable processes...
[ 109.723710] ---[ end Kernel panic - not syncing: Out of memory and no killable processes...
----------
>
> What this patch is trying to handle is a situation when
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM was negotiated. Once
> update_balloon_size_func() started calling fill_balloon(),
> update_balloon_size_func() will continue calling fill_balloon()
> until guest1's memory is reduced to 128MB, won't it?
>
> Since fill_balloon() uses __GFP_IO | __GFP_FS, fill_balloon() can
> indirectly trigger out_of_memory() despite __GFP_NORETRY is specified.
>
> When update_balloon_size_func() is running for calling fill_balloon(),
> calling leak_balloon() will increase number of pages to fill which
> fill_balloon() is supposed to fill. Leaking some pages from leak_balloon()
> via blocking_notifier_call_chain() callback could avoid invocation of the
> OOM killer for that specific moment, but it bounces back to us later because
> number of pages to allocate later (note that update_balloon_size_func() is
> running for calling fill_balloon()) is increased by leak_balloon().
20171016-deflate.log.xz continued printing "puff" messages without any OOM
killer messages, for fill_balloon() always inflates faster than leak_balloon()
deflates.
Since the OOM killer cannot be invoked unless leak_balloon() completely
deflates faster than fill_balloon() inflates, the guest remained unusable
(e.g. unable to login via ssh) other than printing "puff" messages.
This result was worse than 20171016-default.log.xz , for the system was
not able to make any forward progress (i.e. complete OOM lockup).
----------
[ 19.866938] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 20.089350] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 20.430965] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 20.652641] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 20.894680] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 21.122063] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 21.340872] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 21.556128] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 21.772473] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 22.014960] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 24.972961] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 25.201565] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 25.420875] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 25.636081] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 25.851670] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 26.095842] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 26.322284] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 26.851691] virtio_balloon virtio3: Out of puff! Can't get 1 pages
(...snipped...)
[ 211.748567] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 211.963910] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 215.157737] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 215.385239] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 215.604426] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 215.819807] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 216.036491] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 216.278718] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 216.505410] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 216.724334] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 216.940318] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 217.155560] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 220.312187] virtio_balloon virtio3: Out of puff! Can't get 1 pages
[ 220.342980] sysrq: SysRq : Resetting
----------
>
> Thus, I don't think that avoid invoking the OOM killer by calling leak_balloon()
> makes sense when update_balloon_size_func() is running for calling fill_balloon().
> And this patch tries to detect it by replacing mutex_lock() with mutex_trylock().
>
> > Well the point of this flag is that when it's acked,
> > host knows that it's safe to inflate the balloon
> > to a large portion of guest memory and this won't
> > cause an OOM situation.
>
> Assuming that your answer to the question is "no", I don't think it is
> safe to inflate the balloon to a large portion of guest memory, for once
> update_balloon_size_func() started calling fill_balloon(),
> update_balloon_size_func() can not stop calling fill_balloon() even when
> blocking_notifier_call_chain() callback called leak_balloon() because
> "struct virtio_balloon_config"->num_pages will not be updated when
> blocking_notifier_call_chain() callback called leak_balloon().
As I demonstrated above, VIRTIO_BALLOON_F_DEFLATE_ON_OOM can lead to complete
OOM lockup because out_of_memory() => fill_balloon() => out_of_memory() =>
fill_balloon() sequence can effectively disable the OOM killer when the host
assumed that it's safe to inflate the balloon to a large portion of guest
memory and this won't cause an OOM situation.
> > I think the assumption is that it fill back up eventually
> > when guest does have some free memory.
So, my question again because such assumption is broken.
What is the expected behavior after deflating while inflating?
How should "struct virtio_balloon_config"->num_pages be interpreted?
struct virtio_balloon_config {
/* Number of pages host wants Guest to give up. */
__u32 num_pages;
/* Number of pages we've actually got in balloon. */
__u32 actual;
};
If leak_balloon() from out_of_memory() should be stronger than
fill_balloon() from update_balloon_size_func(), we need to make
sure that update_balloon_size_func() stops calling fill_balloon()
when leak_balloon() was called from out_of_memory().
If fill_balloon() from update_balloon_size_func() should be
stronger than leak_balloon() from out_of_memory(), we need to make
sure that leak_balloon() from out_of_memory() is ignored when
fill_balloon() from update_balloon_size_func() is running.
Apart from vb->balloon_lock deadlock avoidance, we need to define
the expected behavior.
^ permalink raw reply
* (unknown),
From: Solen win2 @ 2017-10-15 12:17 UTC (permalink / raw)
To: virtualization
[-- Attachment #1.1: Type: text/plain, Size: 23 bytes --]
Solenwin@freshdesk.com
[-- Attachment #1.2: Type: text/html, Size: 141 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
From: Tetsuo Handa @ 2017-10-15 5:38 UTC (permalink / raw)
To: mst; +Cc: linux-mm, virtualization, mhocko
In-Reply-To: <20171015030921-mutt-send-email-mst@kernel.org>
Michael S. Tsirkin wrote:
> > >
> > > The proper fix isn't that hard - just avoid allocations under lock.
> > >
> > > Patch posted, pls take a look.
> >
> > Your patch allocates pages in order to inflate the balloon, but
> > your patch will allow leak_balloon() to deflate the balloon.
> > How deflating the balloon (i.e. calling leak_balloon()) makes sense
> > when allocating pages for inflating the balloon (i.e. calling
> > fill_balloon()) ?
>
> The idea is that fill_balloon is allocating memory with __GFP_NORETRY
> so it will avoid disruptive actions like the OOM killer.
> Under pressure it will normally fail and retry in half a second or so.
>
> Calling leak_balloon in that situation could benefit the system as a whole.
>
> I might be misunderstanding the meaning of the relevant GFP flags,
> pls correct me if I'm wrong.
Would you answer to below question by "yes"/"no" ?
If leak_balloon() is called via out_of_memory(), leak_balloon()
will decrease "struct virtio_balloon"->num_pages.
But, is "struct virtio_balloon_config"->num_pages updated when
leak_balloon() is called via out_of_memory() ?
Below explanation assumes that your answer is "no".
I consider that fill_balloon() is using __GFP_NORETRY is a bug.
Consider an extreme situation that guest1 is started with 8192MB
memory and then guest1's memory is reduced to 128MB by
virsh qemu-monitor-command --domain guest1 --hmp 'balloon 128'
when VIRTIO_BALLOON_F_DEFLATE_ON_OOM was not negotiated.
Of course, 128MB would be too small to operate guest1 properly.
Since update_balloon_size_func() continues calling fill_balloon()
until guest1's memory is reduced to 128MB, you will see flooding of
"puff" messages (and guest1 is practically unusable because all CPU
resource will be wasted for unsuccessful memory reclaim attempts)
unless the OOM killer is invoked.
What this patch is trying to handle is a situation when
VIRTIO_BALLOON_F_DEFLATE_ON_OOM was negotiated. Once
update_balloon_size_func() started calling fill_balloon(),
update_balloon_size_func() will continue calling fill_balloon()
until guest1's memory is reduced to 128MB, won't it?
Since fill_balloon() uses __GFP_IO | __GFP_FS, fill_balloon() can
indirectly trigger out_of_memory() despite __GFP_NORETRY is specified.
When update_balloon_size_func() is running for calling fill_balloon(),
calling leak_balloon() will increase number of pages to fill which
fill_balloon() is supposed to fill. Leaking some pages from leak_balloon()
via blocking_notifier_call_chain() callback could avoid invocation of the
OOM killer for that specific moment, but it bounces back to us later because
number of pages to allocate later (note that update_balloon_size_func() is
running for calling fill_balloon()) is increased by leak_balloon().
Thus, I don't think that avoid invoking the OOM killer by calling leak_balloon()
makes sense when update_balloon_size_func() is running for calling fill_balloon().
And this patch tries to detect it by replacing mutex_lock() with mutex_trylock().
> Well the point of this flag is that when it's acked,
> host knows that it's safe to inflate the balloon
> to a large portion of guest memory and this won't
> cause an OOM situation.
Assuming that your answer to the question is "no", I don't think it is
safe to inflate the balloon to a large portion of guest memory, for once
update_balloon_size_func() started calling fill_balloon(),
update_balloon_size_func() can not stop calling fill_balloon() even when
blocking_notifier_call_chain() callback called leak_balloon() because
"struct virtio_balloon_config"->num_pages will not be updated when
blocking_notifier_call_chain() callback called leak_balloon().
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Michael S. Tsirkin @ 2017-10-15 0:45 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Willem de Bruijn, netdev, davem, virtualization
In-Reply-To: <20171013155140.124530-1-willemdebruijn.kernel@gmail.com>
On Fri, Oct 13, 2017 at 11:51:40AM -0400, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Implement the reset communication request defined in the VIRTIO 1.0
> specification and introduces in Linux in commit c00bbcf862896 ("virtio:
> add VIRTIO_CONFIG_S_NEEDS_RESET device status bit").
>
> Use the virtnet_reset function introduced in commit 2de2f7f40ef9
> ("virtio_net: XDP support for adjust_head"). That was removed in
> commit 4941d472bf95 ("virtio-net: do not reset during XDP set"),
> because no longer used. Bring it back, minus the xdp specific code.
>
> Before tearing down any state, virtnet_freeze_down quiesces the
> device with netif_tx_disable. virtnet_reset also ensures that no
> other config operations can run concurrently.
>
> On successful reset, the host can observe that the flag has been
> cleared. There is no need for the explicit control flag introduced
> in the previous RFC of this patch.
>
> Changes
> RFC -> v1
> - drop VIRTIO_NET_CTRL_RESET_ACK message
> - drop VIRTIO_F_CAN_RESET flag to notify guest support
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
> drivers/net/virtio_net.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index fc059f193e7d..8e768b54844f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1903,13 +1903,14 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
> .set_link_ksettings = virtnet_set_link_ksettings,
> };
>
> -static void virtnet_freeze_down(struct virtio_device *vdev)
> +static void virtnet_freeze_down(struct virtio_device *vdev, bool in_config)
> {
> struct virtnet_info *vi = vdev->priv;
> int i;
>
> - /* Make sure no work handler is accessing the device */
> - flush_work(&vi->config_work);
> + /* Make sure no other work handler is accessing the device */
> + if (!in_config)
> + flush_work(&vi->config_work);
>
> netif_device_detach(vi->dev);
> netif_tx_disable(vi->dev);
> @@ -1924,6 +1925,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
> }
>
> static int init_vqs(struct virtnet_info *vi);
> +static void remove_vq_common(struct virtnet_info *vi);
>
> static int virtnet_restore_up(struct virtio_device *vdev)
> {
> @@ -1952,6 +1954,40 @@ static int virtnet_restore_up(struct virtio_device *vdev)
> return err;
> }
>
> +static int virtnet_reset(struct virtnet_info *vi)
> +{
> + struct virtio_device *dev = vi->vdev;
> + int ret;
> +
> + virtio_config_disable(dev);
> + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
> + virtnet_freeze_down(dev, true);
> + remove_vq_common(vi);
> +
> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> +
> + ret = virtio_finalize_features(dev);
> + if (ret)
> + goto err;
> +
> + ret = virtnet_restore_up(dev);
> + if (ret)
> + goto err;
> +
> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
> + if (ret)
> + goto err;
> +
> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> + virtio_config_enable(dev);
> + return 0;
> +
> +err:
> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
> + return ret;
> +}
> +
> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
> {
> struct scatterlist sg;
I have a question here though. How do things like MAC address
get restored?
What about the rx mode?
vlans?
Also, it seems that LINK_ANNOUNCE requests will get ignored
even if they got set before the reset, leading to downtime.
> @@ -2136,6 +2172,10 @@ static void virtnet_config_changed_work(struct work_struct *work)
> virtnet_ack_link_announce(vi);
> }
>
> + if (vi->vdev->config->get_status(vi->vdev) &
> + VIRTIO_CONFIG_S_NEEDS_RESET)
> + virtnet_reset(vi);
> +
> /* Ignore unknown (future) status bits */
> v &= VIRTIO_NET_S_LINK_UP;
>
> @@ -2756,7 +2796,7 @@ static __maybe_unused int virtnet_freeze(struct virtio_device *vdev)
> struct virtnet_info *vi = vdev->priv;
>
> virtnet_cpu_notif_remove(vi);
> - virtnet_freeze_down(vdev);
> + virtnet_freeze_down(vdev, false);
> remove_vq_common(vi);
>
> return 0;
> --
> 2.15.0.rc0.271.g36b669edcc-goog
^ permalink raw reply
* Re: [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
From: Michael S. Tsirkin @ 2017-10-15 0:22 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: linux-mm, virtualization, mhocko
In-Reply-To: <201710140141.JFF26087.FLQHOFOOtFMVSJ@I-love.SAKURA.ne.jp>
On Sat, Oct 14, 2017 at 01:41:14AM +0900, Tetsuo Handa wrote:
> Michael S. Tsirkin wrote:
> > On Tue, Oct 10, 2017 at 07:47:37PM +0900, Tetsuo Handa wrote:
> > > In leak_balloon(), mutex_lock(&vb->balloon_lock) is called in order to
> > > serialize against fill_balloon(). But in fill_balloon(),
> > > alloc_page(GFP_HIGHUSER[_MOVABLE] | __GFP_NOMEMALLOC | __GFP_NORETRY) is
> > > called with vb->balloon_lock mutex held. Since GFP_HIGHUSER[_MOVABLE]
> > > implies __GFP_DIRECT_RECLAIM | __GFP_IO | __GFP_FS, despite __GFP_NORETRY
> > > is specified, this allocation attempt might indirectly depend on somebody
> > > else's __GFP_DIRECT_RECLAIM memory allocation. And such indirect
> > > __GFP_DIRECT_RECLAIM memory allocation might call leak_balloon() via
> > > virtballoon_oom_notify() via blocking_notifier_call_chain() callback via
> > > out_of_memory() when it reached __alloc_pages_may_oom() and held oom_lock
> > > mutex. Since vb->balloon_lock mutex is already held by fill_balloon(), it
> > > will cause OOM lockup. Thus, do not wait for vb->balloon_lock mutex if
> > > leak_balloon() is called from out_of_memory().
> > >
> > > Thread1 Thread2
> > > fill_balloon()
> > > takes a balloon_lock
> > > balloon_page_enqueue()
> > > alloc_page(GFP_HIGHUSER_MOVABLE)
> > > direct reclaim (__GFP_FS context) takes a fs lock
> > > waits for that fs lock alloc_page(GFP_NOFS)
> > > __alloc_pages_may_oom()
> > > takes the oom_lock
> > > out_of_memory()
> > > blocking_notifier_call_chain()
> > > leak_balloon()
> > > tries to take that balloon_lock and deadlocks
> > >
> > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >
> > This doesn't deflate on oom if lock is contended, and we acked
> > DEFLATE_ON_OOM so host actually expects us to.
>
> I still don't understand what is wrong with not deflating on OOM.
Well the point of this flag is that when it's acked,
host knows that it's safe to inflate the balloon
to a large portion of guest memory and this won't
cause an OOM situation.
Without this flag, if you inflate after OOM, then it's
fine as allocations fail, but if you inflate and then
enter OOM, balloon won't give up memory.
The flag is unfortunately hard for management tools to use
which led to it still not being supported by hosts.
> According to https://lists.oasis-open.org/archives/virtio-dev/201504/msg00084.html ,
>
> If VIRTIO_BALLOON_F_DEFLATE_ON_OOM has been negotiated, the
> driver MAY use pages from the balloon when \field{num_pages}
> is less than or equal to the actual number of pages in the
> balloon if this is required for system stability
> (e.g. if memory is required by applications running within
> the guest).
>
> it says "MAY" rather than "MUST". I think it is legal to be a no-op.
> Maybe I don't understand the difference between "deflate the balloon" and
> "use pages from the balloon" ?
Maybe we should strengthen this to SHOULD.
> According to https://lists.linuxfoundation.org/pipermail/virtualization/2014-October/027807.html ,
> it seems to me that the expected behavior after deflating while inflating
> was not defined when VIRTIO_BALLOON_F_DEFLATE_ON_OOM was proposed.
I think the assumption is that it fill back up eventually
when guest does have some free memory.
> When the host increased "struct virtio_balloon_config"->num_pages and
> kicked the guest, the guest's update_balloon_size_func() starts calling
> fill_balloon() until "struct virtio_balloon"->num_pages reaches
> "struct virtio_balloon_config"->num_pages, doesn't it?
>
> struct virtio_balloon_config {
> /* Number of pages host wants Guest to give up. */
> __u32 num_pages;
> /* Number of pages we've actually got in balloon. */
> __u32 actual;
> };
>
> If leak_balloon() is called via out_of_memory(), leak_balloon()
> will decrease "struct virtio_balloon"->num_pages.
> But, is "struct virtio_balloon_config"->num_pages updated when
> leak_balloon() is called via out_of_memory() ?
> If yes, update_balloon_size_func() would stop calling fill_balloon()
> when leak_balloon() was called via out_of_memory().
> If no, update_balloon_size_func() would continue calling fill_balloon()
> when leak_balloon() was called via out_of_memory() via fill_balloon()
> via update_balloon_size_func(). That is, when fill_balloon() tries to
> increase "struct virtio_balloon"->num_pages, leak_balloon() which
> decreases "struct virtio_balloon"->num_pages is called due to indirect
> __GFP_DIRECT_RECLAIM dependency via out_of_memory().
> As a result, fill_balloon() will continue trying to increase
> "struct virtio_balloon"->num_pages and leak_balloon() will continue
> decreasing "struct virtio_balloon"->num_pages when leak_balloon()
> is called via fill_balloon() via update_balloon_size_func() due to
> host increased "struct virtio_balloon_config"->num_pages and kicked
> the guest. We deflate the balloon in order to inflate the balloon.
> That is OOM lockup, isn't it? How is such situation better than
> invoking the OOM killer in order to inflate the balloon?
I don't think it's a lockup see below.
> >
> > The proper fix isn't that hard - just avoid allocations under lock.
> >
> > Patch posted, pls take a look.
>
> Your patch allocates pages in order to inflate the balloon, but
> your patch will allow leak_balloon() to deflate the balloon.
> How deflating the balloon (i.e. calling leak_balloon()) makes sense
> when allocating pages for inflating the balloon (i.e. calling
> fill_balloon()) ?
The idea is that fill_balloon is allocating memory with __GFP_NORETRY
so it will avoid disruptive actions like the OOM killer.
Under pressure it will normally fail and retry in half a second or so.
Calling leak_balloon in that situation could benefit the system as a whole.
I might be misunderstanding the meaning of the relevant GFP flags,
pls correct me if I'm wrong.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
From: Tetsuo Handa @ 2017-10-13 16:41 UTC (permalink / raw)
To: mst; +Cc: linux-mm, virtualization, mhocko
In-Reply-To: <20171013162134-mutt-send-email-mst@kernel.org>
Michael S. Tsirkin wrote:
> On Tue, Oct 10, 2017 at 07:47:37PM +0900, Tetsuo Handa wrote:
> > In leak_balloon(), mutex_lock(&vb->balloon_lock) is called in order to
> > serialize against fill_balloon(). But in fill_balloon(),
> > alloc_page(GFP_HIGHUSER[_MOVABLE] | __GFP_NOMEMALLOC | __GFP_NORETRY) is
> > called with vb->balloon_lock mutex held. Since GFP_HIGHUSER[_MOVABLE]
> > implies __GFP_DIRECT_RECLAIM | __GFP_IO | __GFP_FS, despite __GFP_NORETRY
> > is specified, this allocation attempt might indirectly depend on somebody
> > else's __GFP_DIRECT_RECLAIM memory allocation. And such indirect
> > __GFP_DIRECT_RECLAIM memory allocation might call leak_balloon() via
> > virtballoon_oom_notify() via blocking_notifier_call_chain() callback via
> > out_of_memory() when it reached __alloc_pages_may_oom() and held oom_lock
> > mutex. Since vb->balloon_lock mutex is already held by fill_balloon(), it
> > will cause OOM lockup. Thus, do not wait for vb->balloon_lock mutex if
> > leak_balloon() is called from out_of_memory().
> >
> > Thread1 Thread2
> > fill_balloon()
> > takes a balloon_lock
> > balloon_page_enqueue()
> > alloc_page(GFP_HIGHUSER_MOVABLE)
> > direct reclaim (__GFP_FS context) takes a fs lock
> > waits for that fs lock alloc_page(GFP_NOFS)
> > __alloc_pages_may_oom()
> > takes the oom_lock
> > out_of_memory()
> > blocking_notifier_call_chain()
> > leak_balloon()
> > tries to take that balloon_lock and deadlocks
> >
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>
> This doesn't deflate on oom if lock is contended, and we acked
> DEFLATE_ON_OOM so host actually expects us to.
I still don't understand what is wrong with not deflating on OOM.
According to https://lists.oasis-open.org/archives/virtio-dev/201504/msg00084.html ,
If VIRTIO_BALLOON_F_DEFLATE_ON_OOM has been negotiated, the
driver MAY use pages from the balloon when \field{num_pages}
is less than or equal to the actual number of pages in the
balloon if this is required for system stability
(e.g. if memory is required by applications running within
the guest).
it says "MAY" rather than "MUST". I think it is legal to be a no-op.
Maybe I don't understand the difference between "deflate the balloon" and
"use pages from the balloon" ?
According to https://lists.linuxfoundation.org/pipermail/virtualization/2014-October/027807.html ,
it seems to me that the expected behavior after deflating while inflating
was not defined when VIRTIO_BALLOON_F_DEFLATE_ON_OOM was proposed.
When the host increased "struct virtio_balloon_config"->num_pages and
kicked the guest, the guest's update_balloon_size_func() starts calling
fill_balloon() until "struct virtio_balloon"->num_pages reaches
"struct virtio_balloon_config"->num_pages, doesn't it?
struct virtio_balloon_config {
/* Number of pages host wants Guest to give up. */
__u32 num_pages;
/* Number of pages we've actually got in balloon. */
__u32 actual;
};
If leak_balloon() is called via out_of_memory(), leak_balloon()
will decrease "struct virtio_balloon"->num_pages.
But, is "struct virtio_balloon_config"->num_pages updated when
leak_balloon() is called via out_of_memory() ?
If yes, update_balloon_size_func() would stop calling fill_balloon()
when leak_balloon() was called via out_of_memory().
If no, update_balloon_size_func() would continue calling fill_balloon()
when leak_balloon() was called via out_of_memory() via fill_balloon()
via update_balloon_size_func(). That is, when fill_balloon() tries to
increase "struct virtio_balloon"->num_pages, leak_balloon() which
decreases "struct virtio_balloon"->num_pages is called due to indirect
__GFP_DIRECT_RECLAIM dependency via out_of_memory().
As a result, fill_balloon() will continue trying to increase
"struct virtio_balloon"->num_pages and leak_balloon() will continue
decreasing "struct virtio_balloon"->num_pages when leak_balloon()
is called via fill_balloon() via update_balloon_size_func() due to
host increased "struct virtio_balloon_config"->num_pages and kicked
the guest. We deflate the balloon in order to inflate the balloon.
That is OOM lockup, isn't it? How is such situation better than
invoking the OOM killer in order to inflate the balloon?
>
> The proper fix isn't that hard - just avoid allocations under lock.
>
> Patch posted, pls take a look.
Your patch allocates pages in order to inflate the balloon, but
your patch will allow leak_balloon() to deflate the balloon.
How deflating the balloon (i.e. calling leak_balloon()) makes sense
when allocating pages for inflating the balloon (i.e. calling
fill_balloon()) ?
^ permalink raw reply
* [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
From: Willem de Bruijn @ 2017-10-13 15:51 UTC (permalink / raw)
To: netdev; +Cc: Willem de Bruijn, virtualization, davem, mst
From: Willem de Bruijn <willemb@google.com>
Implement the reset communication request defined in the VIRTIO 1.0
specification and introduces in Linux in commit c00bbcf862896 ("virtio:
add VIRTIO_CONFIG_S_NEEDS_RESET device status bit").
Use the virtnet_reset function introduced in commit 2de2f7f40ef9
("virtio_net: XDP support for adjust_head"). That was removed in
commit 4941d472bf95 ("virtio-net: do not reset during XDP set"),
because no longer used. Bring it back, minus the xdp specific code.
Before tearing down any state, virtnet_freeze_down quiesces the
device with netif_tx_disable. virtnet_reset also ensures that no
other config operations can run concurrently.
On successful reset, the host can observe that the flag has been
cleared. There is no need for the explicit control flag introduced
in the previous RFC of this patch.
Changes
RFC -> v1
- drop VIRTIO_NET_CTRL_RESET_ACK message
- drop VIRTIO_F_CAN_RESET flag to notify guest support
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
drivers/net/virtio_net.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index fc059f193e7d..8e768b54844f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1903,13 +1903,14 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
.set_link_ksettings = virtnet_set_link_ksettings,
};
-static void virtnet_freeze_down(struct virtio_device *vdev)
+static void virtnet_freeze_down(struct virtio_device *vdev, bool in_config)
{
struct virtnet_info *vi = vdev->priv;
int i;
- /* Make sure no work handler is accessing the device */
- flush_work(&vi->config_work);
+ /* Make sure no other work handler is accessing the device */
+ if (!in_config)
+ flush_work(&vi->config_work);
netif_device_detach(vi->dev);
netif_tx_disable(vi->dev);
@@ -1924,6 +1925,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
}
static int init_vqs(struct virtnet_info *vi);
+static void remove_vq_common(struct virtnet_info *vi);
static int virtnet_restore_up(struct virtio_device *vdev)
{
@@ -1952,6 +1954,40 @@ static int virtnet_restore_up(struct virtio_device *vdev)
return err;
}
+static int virtnet_reset(struct virtnet_info *vi)
+{
+ struct virtio_device *dev = vi->vdev;
+ int ret;
+
+ virtio_config_disable(dev);
+ dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
+ virtnet_freeze_down(dev, true);
+ remove_vq_common(vi);
+
+ virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
+ virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
+
+ ret = virtio_finalize_features(dev);
+ if (ret)
+ goto err;
+
+ ret = virtnet_restore_up(dev);
+ if (ret)
+ goto err;
+
+ ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
+ if (ret)
+ goto err;
+
+ virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+ virtio_config_enable(dev);
+ return 0;
+
+err:
+ virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
+ return ret;
+}
+
static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
{
struct scatterlist sg;
@@ -2136,6 +2172,10 @@ static void virtnet_config_changed_work(struct work_struct *work)
virtnet_ack_link_announce(vi);
}
+ if (vi->vdev->config->get_status(vi->vdev) &
+ VIRTIO_CONFIG_S_NEEDS_RESET)
+ virtnet_reset(vi);
+
/* Ignore unknown (future) status bits */
v &= VIRTIO_NET_S_LINK_UP;
@@ -2756,7 +2796,7 @@ static __maybe_unused int virtnet_freeze(struct virtio_device *vdev)
struct virtnet_info *vi = vdev->priv;
virtnet_cpu_notif_remove(vi);
- virtnet_freeze_down(vdev);
+ virtnet_freeze_down(vdev, false);
remove_vq_common(vi);
return 0;
--
2.15.0.rc0.271.g36b669edcc-goog
^ permalink raw reply related
* Re: [PATCH] virtio_balloon: fix deadlock on OOM
From: Tetsuo Handa @ 2017-10-13 14:06 UTC (permalink / raw)
To: mst, linux-kernel; +Cc: linux-mm, mhocko, virtualization
In-Reply-To: <1507900754-32239-1-git-send-email-mst@redhat.com>
Michael S. Tsirkin wrote:
> This is a replacement for
> [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
> but unlike that patch it actually deflates on oom even in presence of
> lock contention.
But Wei Wang is proposing VIRTIO_BALLOON_F_SG which will try to allocate
memory, isn't he?
>
> drivers/virtio/virtio_balloon.c | 30 ++++++++++++++++++++++--------
> include/linux/balloon_compaction.h | 38 +++++++++++++++++++++++++++++++++++++-
> mm/balloon_compaction.c | 27 +++++++++++++++++++++------
> 3 files changed, 80 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index f0b3a0b..725e366 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -143,16 +143,14 @@ static void set_page_pfns(struct virtio_balloon *vb,
>
> static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> {
> - struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> unsigned num_allocated_pages;
> + unsigned num_pfns;
> + struct page *page;
> + LIST_HEAD(pages);
>
> - /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> -
I don't think moving this min() to later is correct, for
"num" can be e.g. 1048576, can't it?
> - mutex_lock(&vb->balloon_lock);
> - for (vb->num_pfns = 0; vb->num_pfns < num;
> - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> - struct page *page = balloon_page_enqueue(vb_dev_info);
> + for (num_pfns = 0; num_pfns < num;
> + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> + struct page *page = balloon_page_alloc();
>
> if (!page) {
> dev_info_ratelimited(&vb->vdev->dev,
> @@ -162,6 +160,22 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> msleep(200);
> break;
> }
> +
> + balloon_page_push(&pages, page);
> + }
If balloon_page_alloc() did not fail, it will queue "num"
(e.g. 1048576) pages into pages list, won't it?
> +
> + /* We can only do one array worth at a time. */
> + num = min(num, ARRAY_SIZE(vb->pfns));
> +
Now we cap "num" to VIRTIO_BALLOON_ARRAY_PFNS_MAX (which is 256), but
> + mutex_lock(&vb->balloon_lock);
> +
> + vb->num_pfns = 0;
> +
> + while ((page = balloon_page_pop(&pages))) {
this loop will repeat for e.g. 1048576 times, and
> + balloon_page_enqueue(&vb->vb_dev_info, page);
> +
> + vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
> +
we increment vb->num_pfns for e.g. 1048576 times which will go
beyond VIRTIO_BALLOON_ARRAY_PFNS_MAX array index.
> set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> if (!virtio_has_feature(vb->vdev,
^ permalink raw reply
* Re: [PATCH] virtio_balloon: fix deadlock on OOM
From: Michal Hocko @ 2017-10-13 13:44 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Tetsuo Handa, linux-kernel, virtualization, linux-mm
In-Reply-To: <1507900754-32239-1-git-send-email-mst@redhat.com>
On Fri 13-10-17 16:21:22, Michael S. Tsirkin wrote:
> fill_balloon doing memory allocations under balloon_lock
> can cause a deadlock when leak_balloon is called from
> virtballoon_oom_notify and tries to take same lock.
>
> To fix, split page allocation and enqueue and do allocations outside the lock.
OK, that sounds like a better fix. As long as there are no other
allocations or indirect waiting for an allocation this should work
correctly. Thanks!
> Here's a detailed analysis of the deadlock by Tetsuo Handa:
>
> In leak_balloon(), mutex_lock(&vb->balloon_lock) is called in order to
> serialize against fill_balloon(). But in fill_balloon(),
> alloc_page(GFP_HIGHUSER[_MOVABLE] | __GFP_NOMEMALLOC | __GFP_NORETRY) is
> called with vb->balloon_lock mutex held. Since GFP_HIGHUSER[_MOVABLE]
> implies __GFP_DIRECT_RECLAIM | __GFP_IO | __GFP_FS, despite __GFP_NORETRY
> is specified, this allocation attempt might indirectly depend on somebody
> else's __GFP_DIRECT_RECLAIM memory allocation. And such indirect
> __GFP_DIRECT_RECLAIM memory allocation might call leak_balloon() via
> virtballoon_oom_notify() via blocking_notifier_call_chain() callback via
> out_of_memory() when it reached __alloc_pages_may_oom() and held oom_lock
> mutex. Since vb->balloon_lock mutex is already held by fill_balloon(), it
> will cause OOM lockup. Thus, do not wait for vb->balloon_lock mutex if
> leak_balloon() is called from out_of_memory().
>
> Thread1 Thread2
> fill_balloon()
> takes a balloon_lock
> balloon_page_enqueue()
> alloc_page(GFP_HIGHUSER_MOVABLE)
> direct reclaim (__GFP_FS context) takes a fs lock
> waits for that fs lock alloc_page(GFP_NOFS)
> __alloc_pages_may_oom()
> takes the oom_lock
> out_of_memory()
> blocking_notifier_call_chain()
> leak_balloon()
> tries to take that balloon_lock and deadlocks
>
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Wei Wang <wei.w.wang@intel.com>
> ---
>
> This is a replacement for
> [PATCH] virtio: avoid possible OOM lockup at virtballoon_oom_notify()
> but unlike that patch it actually deflates on oom even in presence of
> lock contention.
>
> drivers/virtio/virtio_balloon.c | 30 ++++++++++++++++++++++--------
> include/linux/balloon_compaction.h | 38 +++++++++++++++++++++++++++++++++++++-
> mm/balloon_compaction.c | 27 +++++++++++++++++++++------
> 3 files changed, 80 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index f0b3a0b..725e366 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -143,16 +143,14 @@ static void set_page_pfns(struct virtio_balloon *vb,
>
> static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> {
> - struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> unsigned num_allocated_pages;
> + unsigned num_pfns;
> + struct page *page;
> + LIST_HEAD(pages);
>
> - /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> -
> - mutex_lock(&vb->balloon_lock);
> - for (vb->num_pfns = 0; vb->num_pfns < num;
> - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> - struct page *page = balloon_page_enqueue(vb_dev_info);
> + for (num_pfns = 0; num_pfns < num;
> + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> + struct page *page = balloon_page_alloc();
>
> if (!page) {
> dev_info_ratelimited(&vb->vdev->dev,
> @@ -162,6 +160,22 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> msleep(200);
> break;
> }
> +
> + balloon_page_push(&pages, page);
> + }
> +
> + /* We can only do one array worth at a time. */
> + num = min(num, ARRAY_SIZE(vb->pfns));
> +
> + mutex_lock(&vb->balloon_lock);
> +
> + vb->num_pfns = 0;
> +
> + while ((page = balloon_page_pop(&pages))) {
> + balloon_page_enqueue(&vb->vb_dev_info, page);
> +
> + vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
> +
> set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> if (!virtio_has_feature(vb->vdev,
> diff --git a/include/linux/balloon_compaction.h b/include/linux/balloon_compaction.h
> index 79542b2..88cfac4 100644
> --- a/include/linux/balloon_compaction.h
> +++ b/include/linux/balloon_compaction.h
> @@ -49,6 +49,7 @@
> #include <linux/gfp.h>
> #include <linux/err.h>
> #include <linux/fs.h>
> +#include <linux/list.h>
>
> /*
> * Balloon device information descriptor.
> @@ -66,9 +67,14 @@ struct balloon_dev_info {
> struct inode *inode;
> };
>
> -extern struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info);
> +extern struct page *balloon_page_alloc(void);
> +extern void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
> + struct page *page);
> extern struct page *balloon_page_dequeue(struct balloon_dev_info *b_dev_info);
>
> +extern void balloon_devinfo_splice(struct balloon_dev_info *to_add,
> + struct balloon_dev_info *b_dev_info);
> +
> static inline void balloon_devinfo_init(struct balloon_dev_info *balloon)
> {
> balloon->isolated_pages = 0;
> @@ -88,6 +94,36 @@ extern int balloon_page_migrate(struct address_space *mapping,
> struct page *page, enum migrate_mode mode);
>
> /*
> + * balloon_page_push - insert a page into a page list.
> + * @head : pointer to list
> + * @page : page to be added
> + *
> + * Caller must ensure the page is private and protect the list.
> + */
> +static inline void balloon_page_push(struct list_head *pages, struct page *page)
> +{
> + list_add(&page->lru, pages);
> +}
> +
> +/*
> + * balloon_page_pop - remove a page from a page list.
> + * @head : pointer to list
> + * @page : page to be added
> + *
> + * Caller must ensure the page is private and protect the list.
> + */
> +static inline struct page *balloon_page_pop(struct list_head *pages)
> +{
> + struct page *page = list_first_entry_or_null(pages, struct page, lru);
> +
> + if (!page)
> + return NULL;
> +
> + list_del(&page->lru);
> + return page;
> +}
> +
> +/*
> * balloon_page_insert - insert a page into the balloon's page list and make
> * the page->private assignment accordingly.
> * @balloon : pointer to balloon device
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index b06d9fe..cd605bf 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -11,22 +11,37 @@
> #include <linux/balloon_compaction.h>
>
> /*
> + * balloon_page_alloc - allocates a new page for insertion into the balloon
> + * page list.
> + *
> + * Driver must call it to properly allocate a new enlisted balloon page.
> + * Driver must call balloon_page_enqueue before definitively removing it from
> + * the guest system. This function returns the page address for the recently
> + * allocated page or NULL in the case we fail to allocate a new page this turn.
> + */
> +struct page *balloon_page_alloc(void)
> +{
> + struct page *page = alloc_page(balloon_mapping_gfp_mask() |
> + __GFP_NOMEMALLOC | __GFP_NORETRY);
> + return page;
> +}
> +EXPORT_SYMBOL_GPL(balloon_page_alloc);
> +
> +/*
> * balloon_page_enqueue - allocates a new page and inserts it into the balloon
> * page list.
> * @b_dev_info: balloon device descriptor where we will insert a new page to
> + * @page: new page to enqueue - allocated using balloon_page_alloc.
> *
> - * Driver must call it to properly allocate a new enlisted balloon page
> + * Driver must call it to properly enqueue a new allocated balloon page
> * before definitively removing it from the guest system.
> * This function returns the page address for the recently enqueued page or
> * NULL in the case we fail to allocate a new page this turn.
> */
> -struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info)
> +void balloon_page_enqueue(struct balloon_dev_info *b_dev_info,
> + struct page *page)
> {
> unsigned long flags;
> - struct page *page = alloc_page(balloon_mapping_gfp_mask() |
> - __GFP_NOMEMALLOC | __GFP_NORETRY);
> - if (!page)
> - return NULL;
>
> /*
> * Block others from accessing the 'page' when we get around to
> --
> MST
--
Michal Hocko
SUSE Labs
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox