[PATCH] ib_srp: Let compiler ignore the useless debug code

[Minfei Huang]

"if (0)" is used to make this block of debug code not be executed. There
is a more elegant way to let compiler ignore this code, using
"#if 0 .. #endif" instead.

Although it may be optimised by some compilers with specified parameter,
just for readable.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index 36529e3..f2d7821 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -2023,12 +2023,12 @@ static void srp_recv_done(struct ib_cq *cq, struct 
ib_wc *wc)
 
opcode = *(u8 *) iu->buf;
 
-   if (0) {
-   shost_printk(KERN_ERR, target->scsi_host,
-PFX "recv completion, opcode 0x%02x\n", opcode);
-   print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 8, 1,
-  iu->buf, wc->byte_len, true);
-   }
+#if 0
+   shost_printk(KERN_ERR, target->scsi_host,
+   PFX "recv completion, opcode 0x%02x\n", opcode);
+   print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 8, 1,
+   iu->buf, wc->byte_len, true);
+#endif
 
switch (opcode) {
case SRP_RSP:
-- 
2.10.1 (Apple Git-78)

[PATCH] ib_srp: Let compiler ignore the useless debug code

[Minfei Huang]

"if (0)" is used to make this block of debug code not be executed. There
is a more elegant way to let compiler ignore this code, using
"#if 0 .. #endif" instead.

Although it may be optimised by some compilers with specified parameter,
just for readable.

Signed-off-by: Minfei Huang 
---
 drivers/infiniband/ulp/srp/ib_srp.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index 36529e3..f2d7821 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -2023,12 +2023,12 @@ static void srp_recv_done(struct ib_cq *cq, struct 
ib_wc *wc)
 
opcode = *(u8 *) iu->buf;
 
-   if (0) {
-   shost_printk(KERN_ERR, target->scsi_host,
-PFX "recv completion, opcode 0x%02x\n", opcode);
-   print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 8, 1,
-  iu->buf, wc->byte_len, true);
-   }
+#if 0
+   shost_printk(KERN_ERR, target->scsi_host,
+   PFX "recv completion, opcode 0x%02x\n", opcode);
+   print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 8, 1,
+   iu->buf, wc->byte_len, true);
+#endif
 
switch (opcode) {
case SRP_RSP:
-- 
2.10.1 (Apple Git-78)

Re: [PATCH] dm: Return correct value in retry loop

[Minfei Huang]

Ping.

Any comment is appreciate.

Thanks
Minfei

On 09/06/16 at 04:00P, Minfei Huang wrote:
> dm_resume will return sliently in retry loop's failure. Assign a correct
> return value in the failed loop.
> 
> Remove a useless assignment as well.
> 
> Signed-off-by: Minfei Huang <mngh...@gmail.com>
> ---
>  drivers/md/dm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index fa9b1cb..c935cc8 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -2249,10 +2249,11 @@ static int __dm_resume(struct mapped_device *md, 
> struct dm_table *map)
>  
>  int dm_resume(struct mapped_device *md)
>  {
> - int r = -EINVAL;
> + int r;
>   struct dm_table *map = NULL;
>  
>  retry:
> + r = -EINVAL;
>   mutex_lock_nested(>suspend_lock, SINGLE_DEPTH_NESTING);
>  
>   if (!dm_suspended_md(md))
> @@ -2277,10 +2278,8 @@ retry:
>  
>   clear_bit(DMF_SUSPENDED, >flags);
>  
> - r = 0;
>  out:
>   mutex_unlock(>suspend_lock);
> -
>   return r;
>  }
>  
> -- 
> 2.7.4 (Apple Git-66)
> 

Re: [PATCH] dm: Return correct value in retry loop

[Minfei Huang]

Ping.

Any comment is appreciate.

Thanks
Minfei

On 09/06/16 at 04:00P, Minfei Huang wrote:
> dm_resume will return sliently in retry loop's failure. Assign a correct
> return value in the failed loop.
> 
> Remove a useless assignment as well.
> 
> Signed-off-by: Minfei Huang 
> ---
>  drivers/md/dm.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index fa9b1cb..c935cc8 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -2249,10 +2249,11 @@ static int __dm_resume(struct mapped_device *md, 
> struct dm_table *map)
>  
>  int dm_resume(struct mapped_device *md)
>  {
> - int r = -EINVAL;
> + int r;
>   struct dm_table *map = NULL;
>  
>  retry:
> + r = -EINVAL;
>   mutex_lock_nested(>suspend_lock, SINGLE_DEPTH_NESTING);
>  
>   if (!dm_suspended_md(md))
> @@ -2277,10 +2278,8 @@ retry:
>  
>   clear_bit(DMF_SUSPENDED, >flags);
>  
> - r = 0;
>  out:
>   mutex_unlock(>suspend_lock);
> -
>   return r;
>  }
>  
> -- 
> 2.7.4 (Apple Git-66)
> 

[PATCH] dm: Return correct value in retry loop

[Minfei Huang]

dm_resume will return sliently in retry loop's failure. Assign a correct
return value in the failed loop.

Remove a useless assignment as well.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 drivers/md/dm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index fa9b1cb..c935cc8 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2249,10 +2249,11 @@ static int __dm_resume(struct mapped_device *md, struct 
dm_table *map)
 
 int dm_resume(struct mapped_device *md)
 {
-   int r = -EINVAL;
+   int r;
struct dm_table *map = NULL;
 
 retry:
+   r = -EINVAL;
mutex_lock_nested(>suspend_lock, SINGLE_DEPTH_NESTING);
 
if (!dm_suspended_md(md))
@@ -2277,10 +2278,8 @@ retry:
 
clear_bit(DMF_SUSPENDED, >flags);
 
-   r = 0;
 out:
mutex_unlock(>suspend_lock);
-
return r;
 }
 
-- 
2.7.4 (Apple Git-66)

[PATCH] dm: Return correct value in retry loop

[Minfei Huang]

dm_resume will return sliently in retry loop's failure. Assign a correct
return value in the failed loop.

Remove a useless assignment as well.

Signed-off-by: Minfei Huang 
---
 drivers/md/dm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index fa9b1cb..c935cc8 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2249,10 +2249,11 @@ static int __dm_resume(struct mapped_device *md, struct 
dm_table *map)
 
 int dm_resume(struct mapped_device *md)
 {
-   int r = -EINVAL;
+   int r;
struct dm_table *map = NULL;
 
 retry:
+   r = -EINVAL;
mutex_lock_nested(>suspend_lock, SINGLE_DEPTH_NESTING);
 
if (!dm_suspended_md(md))
@@ -2277,10 +2278,8 @@ retry:
 
clear_bit(DMF_SUSPENDED, >flags);
 
-   r = 0;
 out:
mutex_unlock(>suspend_lock);
-
return r;
 }
 
-- 
2.7.4 (Apple Git-66)

[PATCH RESEND] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

We do a lot of memory allocation in function init_vq, and don't handle
the allocation failure properly. Then this function will return 0,
although initialization fails due to lacking memory. At that moment,
kernel will panic in guest machine, if virtio is used to drive disk.

To fix this bug, we should take care of allocation failure, and return
correct value to let caller know what happen.

Tested-by: Chao Fan <fanc.f...@cn.fujitsu.com>
Signed-off-by: Minfei Huang <mngh...@gmail.com>
Signed-off-by: Minfei Huang <minfei@alibaba-inc.com>
Reviewed-by: Cornelia Huck <cornelia.h...@de.ibm.com>
Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
---
 drivers/block/virtio_blk.c | 26 --
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 1523e05..93b1aaa 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -391,22 +391,16 @@ static int init_vq(struct virtio_blk *vblk)
num_vqs = 1;
 
vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
-   if (!vblk->vqs) {
-   err = -ENOMEM;
-   goto out;
-   }
+   if (!vblk->vqs)
+   return -ENOMEM;
 
names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
-   if (!names)
-   goto err_names;
-
callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
-   if (!callbacks)
-   goto err_callbacks;
-
vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
-   if (!vqs)
-   goto err_vqs;
+   if (!names || !callbacks || !vqs) {
+   err = -ENOMEM;
+   goto out;
+   }
 
for (i = 0; i < num_vqs; i++) {
callbacks[i] = virtblk_done;
@@ -417,7 +411,7 @@ static int init_vq(struct virtio_blk *vblk)
/* Discover virtqueues and write information to configuration.  */
err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
if (err)
-   goto err_find_vqs;
+   goto out;
 
for (i = 0; i < num_vqs; i++) {
spin_lock_init(>vqs[i].lock);
@@ -425,16 +419,12 @@ static int init_vq(struct virtio_blk *vblk)
}
vblk->num_vqs = num_vqs;
 
- err_find_vqs:
+out:
kfree(vqs);
- err_vqs:
kfree(callbacks);
- err_callbacks:
kfree(names);
- err_names:
if (err)
kfree(vblk->vqs);
- out:
return err;
 }
 
-- 
2.7.4 (Apple Git-66)

[PATCH RESEND] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

We do a lot of memory allocation in function init_vq, and don't handle
the allocation failure properly. Then this function will return 0,
although initialization fails due to lacking memory. At that moment,
kernel will panic in guest machine, if virtio is used to drive disk.

To fix this bug, we should take care of allocation failure, and return
correct value to let caller know what happen.

Tested-by: Chao Fan 
Signed-off-by: Minfei Huang 
Signed-off-by: Minfei Huang 
Reviewed-by: Cornelia Huck 
Reviewed-by: Stefan Hajnoczi 
---
 drivers/block/virtio_blk.c | 26 --
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 1523e05..93b1aaa 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -391,22 +391,16 @@ static int init_vq(struct virtio_blk *vblk)
num_vqs = 1;
 
vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
-   if (!vblk->vqs) {
-   err = -ENOMEM;
-   goto out;
-   }
+   if (!vblk->vqs)
+   return -ENOMEM;
 
names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
-   if (!names)
-   goto err_names;
-
callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
-   if (!callbacks)
-   goto err_callbacks;
-
vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
-   if (!vqs)
-   goto err_vqs;
+   if (!names || !callbacks || !vqs) {
+   err = -ENOMEM;
+   goto out;
+   }
 
for (i = 0; i < num_vqs; i++) {
callbacks[i] = virtblk_done;
@@ -417,7 +411,7 @@ static int init_vq(struct virtio_blk *vblk)
/* Discover virtqueues and write information to configuration.  */
err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
if (err)
-   goto err_find_vqs;
+   goto out;
 
for (i = 0; i < num_vqs; i++) {
spin_lock_init(>vqs[i].lock);
@@ -425,16 +419,12 @@ static int init_vq(struct virtio_blk *vblk)
}
vblk->num_vqs = num_vqs;
 
- err_find_vqs:
+out:
kfree(vqs);
- err_vqs:
kfree(callbacks);
- err_callbacks:
kfree(names);
- err_names:
if (err)
kfree(vblk->vqs);
- out:
return err;
 }
 
-- 
2.7.4 (Apple Git-66)

Re: [PATCH v3] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

Hi, Michael.

Since Stefan and Cornelia have review-acked this patch, could you mind
helping review this patch?

Thanks
Minfei

> On Jul 29, 2016, at 16:26, Stefan Hajnoczi <stefa...@gmail.com> wrote:
> 
> On Tue, Jul 19, 2016 at 5:32 AM, Minfei Huang <mnfhu...@gmail.com> wrote:
>> From: Minfei Huang <mngh...@gmail.com>
>> 
>> We do a lot of memory allocation in function init_vq, and don't handle
>> the allocation failure properly. Then this function will return 0,
>> although initialization fails due to lacking memory. At that moment,
>> kernel will panic in guest machine, if virtio is used to drive disk.
>> 
>> To fix this bug, we should take care of allocation failure, and return
>> correct value to let caller know what happen.
>> 
>> Tested-by: Chao Fan <fanc.f...@cn.fujitsu.com>
>> Signed-off-by: Minfei Huang <minfei@alibaba-inc.com>
>> Signed-off-by: Minfei Huang <mngh...@gmail.com>
>> ---
>> v2:
>> - Remove useless initialisation to NULL
>> v1:
>> - Refactor the patch to make code more readable
>> ---
>> drivers/block/virtio_blk.c | 26 --
>> 1 file changed, 8 insertions(+), 18 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v3] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

Hi, Michael.

Since Stefan and Cornelia have review-acked this patch, could you mind
helping review this patch?

Thanks
Minfei

> On Jul 29, 2016, at 16:26, Stefan Hajnoczi  wrote:
> 
> On Tue, Jul 19, 2016 at 5:32 AM, Minfei Huang  wrote:
>> From: Minfei Huang 
>> 
>> We do a lot of memory allocation in function init_vq, and don't handle
>> the allocation failure properly. Then this function will return 0,
>> although initialization fails due to lacking memory. At that moment,
>> kernel will panic in guest machine, if virtio is used to drive disk.
>> 
>> To fix this bug, we should take care of allocation failure, and return
>> correct value to let caller know what happen.
>> 
>> Tested-by: Chao Fan 
>> Signed-off-by: Minfei Huang 
>> Signed-off-by: Minfei Huang 
>> ---
>> v2:
>> - Remove useless initialisation to NULL
>> v1:
>> - Refactor the patch to make code more readable
>> ---
>> drivers/block/virtio_blk.c | 26 --
>> 1 file changed, 8 insertions(+), 18 deletions(-)
> 
> Reviewed-by: Stefan Hajnoczi 



smime.p7s
Description: S/MIME cryptographic signature

Re: [lkp] [blk] ee5c4fef9f: BUG: unable to handle kernel NULL pointer dereference at 0000010b

[Minfei Huang]

Hi, Xiaolong.

I think it’s the correct behaviour for my patch to handle bio, and there is 
something
wrong with floppy driver. I will post a patch to fix this floppy’s bug soon.

Thanks
Minfei

> On Jul 29, 2016, at 10:21, kernel test robot <xiaolong...@intel.com> wrote:
> 
> 
> FYI, we noticed the following commit:
> 
> https://github.com/0day-ci/linux 
> Minfei-Huang/blk-core-Fix-the-bad-IO-during-checking-bio/20160728-182758
> commit ee5c4fef9f2ef03ee8f283a5b24192df00e17f0f ("blk-core: Fix the bad IO 
> during checking bio")
> 
> in testcase: boot
> 
> on test machine: 2 threads qemu-system-i386 -enable-kvm with 320M memory
> 
> caused below changes:
> 
> 
> ++++
> || b013517951 | ee5c4fef9f |
> ++++
> | boot_successes | 11 | 2  |
> | boot_failures  | 1  | 10 |
> | BUG:kernel_test_crashed| 1  ||
> | BUG:unable_to_handle_kernel| 0  | 8  |
> | Oops   | 0  | 8  |
> | EIP_is_at__lock_acquire| 0  | 8  |
> | Kernel_panic-not_syncing:Fatal_exception   | 0  | 8  |
> | IP-Config:Auto-configuration_of_network_failed | 0  | 2  |
> ++++
> 
> 
> 
> [   24.378591] attempt to access beyond end of device
> [   24.378593] fd0: rw=0, want=8, limit=0
> [   24.378594] floppy: error -5 while reading block 0
> [   24.378600] BUG: unable to handle kernel NULL pointer dereference at 
> 010b
> [   24.378605] IP: [<7906d275>] __lock_acquire+0xa7/0x612
> [   24.378606] *pde =  
> [   24.378608] Oops: 0002 [#1] SMP
> [   24.378611] CPU: 1 PID: 574 Comm: mount Not tainted 
> 4.7.0-rc2-00241-gee5c4fe #4
> [   24.378612] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> Debian-1.8.2-1 04/01/2014
> [   24.378614] task: 87152c80 ti: 883f task.ti: 883f
> [   24.378615] EIP: 0060:[<7906d275>] EFLAGS: 00010002 CPU: 1
> [   24.378617] EIP is at __lock_acquire+0xa7/0x612
> [   24.378618] EAX: 0007 EBX: 0002 ECX:  EDX: 
> [   24.378619] ESI: 0001 EDI: 87152c80 EBP: 883f1c2c ESP: 883f1c00
> [   24.378620]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> [   24.378621] CR0: 80050033 CR2: 010b CR3: 0f8bd000 CR4: 0690
> [   24.378625] Stack:
> [   24.378630]   7a267440 0202 883f1c1c   
> 883f1d74 883f1c2c
> [   24.378634]  0002 87152c80 883f1d74 883f1c64 7906da8d  
> 0001 0001
> [   24.378637]   79066107     
> 883f1d64 0202
> [   24.378638] Call Trace:
> [   24.378640]  [<7906da8d>] lock_acquire+0x60/0x7c
> [   24.378644]  [<79066107>] ? complete+0x12/0x35
> [   24.378648]  [<79b9a42a>] _raw_spin_lock_irqsave+0x34/0x44
> [   24.378650]  [<79066107>] ? complete+0x12/0x35
> [   24.378651]  [<79066107>] complete+0x12/0x35
> [   24.378654]  [<79467b9a>] floppy_rb0_cb+0x31/0x38
> [   24.378656]  [<7932d102>] bio_endio+0x39/0x51
> [   24.378659]  [<7932ec47>] generic_make_request_checks+0x13a/0x144
> [   24.378661]  [<793300ae>] generic_make_request+0x11/0x12a
> [   24.378663]  [<79330293>] submit_bio+0xcc/0xd3
> [   24.378665]  [<79468347>] __floppy_read_block_0+0xbc/0xfe
> [   24.378668]  [<7906bfa3>] ? mark_held_locks+0x4b/0x65
> [   24.378671]  [<79b9a5de>] ? _raw_spin_unlock_irqrestore+0x39/0x4b
> [   24.378672]  [<79467b69>] ? floppy_find+0x3b/0x3b
> [   24.378674]  [<79468955>] floppy_revalidate+0x104/0x171
> [   24.378678]  [<79117276>] check_disk_change+0x41/0x4e
> [   24.378680]  [<79467e9a>] floppy_open+0x20c/0x28d
> [   24.378682]  [<7911697b>] __blkdev_get+0xf9/0x34f
> [   24.378684]  [<79116d39>] blkdev_get+0x168/0x25c
> [   24.378689]  [<790f8206>] ? path_put+0x15/0x18
> [   24.378691]  [<79117061>] ? lookup_bdev+0x62/0x72
> [   24.378693]  [<79117094>] blkdev_get_by_path+0x23/0x53
> [   24.378696]  [<790f2820>] mount_bdev+0x2a/0x157
> [   24.378700]  [<7917748a>] ext4_mount+0x10/0x12
> [   24.378702]  [<7917af40>] ? ext4_calculate_overhead+0x30e/0x30e
> [   24.378704]  [<790f2ad3>] mount_fs+0x53/0x110
> [   24.378708]  [<79107ab4>] vfs_kern_mount+0x47/0xaa
> [   24.378710]

Re: [lkp] [blk] ee5c4fef9f: BUG: unable to handle kernel NULL pointer dereference at 0000010b

[Minfei Huang]

Hi, Xiaolong.

I think it’s the correct behaviour for my patch to handle bio, and there is 
something
wrong with floppy driver. I will post a patch to fix this floppy’s bug soon.

Thanks
Minfei

> On Jul 29, 2016, at 10:21, kernel test robot  wrote:
> 
> 
> FYI, we noticed the following commit:
> 
> https://github.com/0day-ci/linux 
> Minfei-Huang/blk-core-Fix-the-bad-IO-during-checking-bio/20160728-182758
> commit ee5c4fef9f2ef03ee8f283a5b24192df00e17f0f ("blk-core: Fix the bad IO 
> during checking bio")
> 
> in testcase: boot
> 
> on test machine: 2 threads qemu-system-i386 -enable-kvm with 320M memory
> 
> caused below changes:
> 
> 
> ++++
> || b013517951 | ee5c4fef9f |
> ++++
> | boot_successes | 11 | 2  |
> | boot_failures  | 1  | 10 |
> | BUG:kernel_test_crashed| 1  ||
> | BUG:unable_to_handle_kernel| 0  | 8  |
> | Oops   | 0  | 8  |
> | EIP_is_at__lock_acquire| 0  | 8  |
> | Kernel_panic-not_syncing:Fatal_exception   | 0  | 8  |
> | IP-Config:Auto-configuration_of_network_failed | 0  | 2  |
> ++++
> 
> 
> 
> [   24.378591] attempt to access beyond end of device
> [   24.378593] fd0: rw=0, want=8, limit=0
> [   24.378594] floppy: error -5 while reading block 0
> [   24.378600] BUG: unable to handle kernel NULL pointer dereference at 
> 010b
> [   24.378605] IP: [<7906d275>] __lock_acquire+0xa7/0x612
> [   24.378606] *pde =  
> [   24.378608] Oops: 0002 [#1] SMP
> [   24.378611] CPU: 1 PID: 574 Comm: mount Not tainted 
> 4.7.0-rc2-00241-gee5c4fe #4
> [   24.378612] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> Debian-1.8.2-1 04/01/2014
> [   24.378614] task: 87152c80 ti: 883f task.ti: 883f
> [   24.378615] EIP: 0060:[<7906d275>] EFLAGS: 00010002 CPU: 1
> [   24.378617] EIP is at __lock_acquire+0xa7/0x612
> [   24.378618] EAX: 0007 EBX: 0002 ECX:  EDX: 
> [   24.378619] ESI: 0001 EDI: 87152c80 EBP: 883f1c2c ESP: 883f1c00
> [   24.378620]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> [   24.378621] CR0: 80050033 CR2: 010b CR3: 0f8bd000 CR4: 0690
> [   24.378625] Stack:
> [   24.378630]   7a267440 0202 883f1c1c   
> 883f1d74 883f1c2c
> [   24.378634]  0002 87152c80 883f1d74 883f1c64 7906da8d  
> 0001 0001
> [   24.378637]   79066107     
> 883f1d64 0202
> [   24.378638] Call Trace:
> [   24.378640]  [<7906da8d>] lock_acquire+0x60/0x7c
> [   24.378644]  [<79066107>] ? complete+0x12/0x35
> [   24.378648]  [<79b9a42a>] _raw_spin_lock_irqsave+0x34/0x44
> [   24.378650]  [<79066107>] ? complete+0x12/0x35
> [   24.378651]  [<79066107>] complete+0x12/0x35
> [   24.378654]  [<79467b9a>] floppy_rb0_cb+0x31/0x38
> [   24.378656]  [<7932d102>] bio_endio+0x39/0x51
> [   24.378659]  [<7932ec47>] generic_make_request_checks+0x13a/0x144
> [   24.378661]  [<793300ae>] generic_make_request+0x11/0x12a
> [   24.378663]  [<79330293>] submit_bio+0xcc/0xd3
> [   24.378665]  [<79468347>] __floppy_read_block_0+0xbc/0xfe
> [   24.378668]  [<7906bfa3>] ? mark_held_locks+0x4b/0x65
> [   24.378671]  [<79b9a5de>] ? _raw_spin_unlock_irqrestore+0x39/0x4b
> [   24.378672]  [<79467b69>] ? floppy_find+0x3b/0x3b
> [   24.378674]  [<79468955>] floppy_revalidate+0x104/0x171
> [   24.378678]  [<79117276>] check_disk_change+0x41/0x4e
> [   24.378680]  [<79467e9a>] floppy_open+0x20c/0x28d
> [   24.378682]  [<7911697b>] __blkdev_get+0xf9/0x34f
> [   24.378684]  [<79116d39>] blkdev_get+0x168/0x25c
> [   24.378689]  [<790f8206>] ? path_put+0x15/0x18
> [   24.378691]  [<79117061>] ? lookup_bdev+0x62/0x72
> [   24.378693]  [<79117094>] blkdev_get_by_path+0x23/0x53
> [   24.378696]  [<790f2820>] mount_bdev+0x2a/0x157
> [   24.378700]  [<7917748a>] ext4_mount+0x10/0x12
> [   24.378702]  [<7917af40>] ? ext4_calculate_overhead+0x30e/0x30e
> [   24.378704]  [<790f2ad3>] mount_fs+0x53/0x110
> [   24.378708]  [<79107ab4>] vfs_kern_mount+0x47/0xaa
> [   24.378710]  [<79108d9b>] do_mou

[PATCH] blk-core: Fix the bad IO during checking bio

[Minfei Huang]

We will handle IO as a bad IO, if its range exceeds the disk's capacity
in function bio_check_eod. Also we should catch the corner case if
inode->i_size is set to 0 during disabling disk.

Fix the coming IO as a bad IO as well, if inode->i_size is set to 0.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
Signed-off-by: Minfei Huang <minfei@alibaba-inc.com>
---
 block/blk-core.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2475b1c7..765dfc4 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1896,25 +1896,23 @@ static inline bool should_fail_request(struct hd_struct 
*part,
  */
 static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
 {
-   sector_t maxsector;
+   sector_t maxsector, sector;
 
if (!nr_sectors)
return 0;
 
/* Test device or partition size, when known. */
maxsector = i_size_read(bio->bi_bdev->bd_inode) >> 9;
-   if (maxsector) {
-   sector_t sector = bio->bi_iter.bi_sector;
+   sector = bio->bi_iter.bi_sector;
 
-   if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
-   /*
-* This may well happen - the kernel calls bread()
-* without checking the size of the device, e.g., when
-* mounting a device.
-*/
-   handle_bad_sector(bio);
-   return 1;
-   }
+   if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
+   /*
+* This may well happen - the kernel calls bread()
+* without checking the size of the device, e.g., when
+* mounting a device.
+*/
+   handle_bad_sector(bio);
+   return 1;
}
 
return 0;
-- 
2.7.4 (Apple Git-66)

[PATCH] blk-core: Fix the bad IO during checking bio

[Minfei Huang]

We will handle IO as a bad IO, if its range exceeds the disk's capacity
in function bio_check_eod. Also we should catch the corner case if
inode->i_size is set to 0 during disabling disk.

Fix the coming IO as a bad IO as well, if inode->i_size is set to 0.

Signed-off-by: Minfei Huang 
Signed-off-by: Minfei Huang 
---
 block/blk-core.c | 22 ++
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2475b1c7..765dfc4 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1896,25 +1896,23 @@ static inline bool should_fail_request(struct hd_struct 
*part,
  */
 static inline int bio_check_eod(struct bio *bio, unsigned int nr_sectors)
 {
-   sector_t maxsector;
+   sector_t maxsector, sector;
 
if (!nr_sectors)
return 0;
 
/* Test device or partition size, when known. */
maxsector = i_size_read(bio->bi_bdev->bd_inode) >> 9;
-   if (maxsector) {
-   sector_t sector = bio->bi_iter.bi_sector;
+   sector = bio->bi_iter.bi_sector;
 
-   if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
-   /*
-* This may well happen - the kernel calls bread()
-* without checking the size of the device, e.g., when
-* mounting a device.
-*/
-   handle_bad_sector(bio);
-   return 1;
-   }
+   if (maxsector < nr_sectors || maxsector - nr_sectors < sector) {
+   /*
+* This may well happen - the kernel calls bread()
+* without checking the size of the device, e.g., when
+* mounting a device.
+*/
+   handle_bad_sector(bio);
+   return 1;
}
 
return 0;
-- 
2.7.4 (Apple Git-66)

Re: [PATCH v3] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

Ping, Any comment is appreciate.

Thanks
Minfei

> On Jul 19, 2016, at 20:22, Cornelia Huck <cornelia.h...@de.ibm.com> wrote:
> 
> On Tue, 19 Jul 2016 12:32:42 +0800
> Minfei Huang <mnfhu...@gmail.com> wrote:
> 
>> From: Minfei Huang <mngh...@gmail.com>
>> 
>> We do a lot of memory allocation in function init_vq, and don't handle
>> the allocation failure properly. Then this function will return 0,
>> although initialization fails due to lacking memory. At that moment,
>> kernel will panic in guest machine, if virtio is used to drive disk.
>> 
>> To fix this bug, we should take care of allocation failure, and return
>> correct value to let caller know what happen.
>> 
>> Tested-by: Chao Fan <fanc.f...@cn.fujitsu.com>
>> Signed-off-by: Minfei Huang <minfei@alibaba-inc.com>
>> Signed-off-by: Minfei Huang <mngh...@gmail.com>
>> ---
>> v2:
>> - Remove useless initialisation to NULL
>> v1:
>> - Refactor the patch to make code more readable
>> ---
>> drivers/block/virtio_blk.c | 26 --
>> 1 file changed, 8 insertions(+), 18 deletions(-)
> 
> Your changes certainly make the function more compact.
> 
> Reviewed-by: Cornelia Huck <cornelia.h...@de.ibm.com>



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v3] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

Ping, Any comment is appreciate.

Thanks
Minfei

> On Jul 19, 2016, at 20:22, Cornelia Huck  wrote:
> 
> On Tue, 19 Jul 2016 12:32:42 +0800
> Minfei Huang  wrote:
> 
>> From: Minfei Huang 
>> 
>> We do a lot of memory allocation in function init_vq, and don't handle
>> the allocation failure properly. Then this function will return 0,
>> although initialization fails due to lacking memory. At that moment,
>> kernel will panic in guest machine, if virtio is used to drive disk.
>> 
>> To fix this bug, we should take care of allocation failure, and return
>> correct value to let caller know what happen.
>> 
>> Tested-by: Chao Fan 
>> Signed-off-by: Minfei Huang 
>> Signed-off-by: Minfei Huang 
>> ---
>> v2:
>> - Remove useless initialisation to NULL
>> v1:
>> - Refactor the patch to make code more readable
>> ---
>> drivers/block/virtio_blk.c | 26 --
>> 1 file changed, 8 insertions(+), 18 deletions(-)
> 
> Your changes certainly make the function more compact.
> 
> Reviewed-by: Cornelia Huck 



smime.p7s
Description: S/MIME cryptographic signature

[PATCH v3] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

From: Minfei Huang <mngh...@gmail.com>

We do a lot of memory allocation in function init_vq, and don't handle
the allocation failure properly. Then this function will return 0,
although initialization fails due to lacking memory. At that moment,
kernel will panic in guest machine, if virtio is used to drive disk.

To fix this bug, we should take care of allocation failure, and return
correct value to let caller know what happen.

Tested-by: Chao Fan <fanc.f...@cn.fujitsu.com>
Signed-off-by: Minfei Huang <minfei@alibaba-inc.com>
Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
v2:
- Remove useless initialisation to NULL
v1:
- Refactor the patch to make code more readable
---
 drivers/block/virtio_blk.c | 26 --
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 42758b5..4ee78c0 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -394,22 +394,16 @@ static int init_vq(struct virtio_blk *vblk)
num_vqs = 1;
 
vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
-   if (!vblk->vqs) {
-   err = -ENOMEM;
-   goto out;
-   }
+   if (!vblk->vqs)
+   return -ENOMEM;
 
names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
-   if (!names)
-   goto err_names;
-
callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
-   if (!callbacks)
-   goto err_callbacks;
-
vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
-   if (!vqs)
-   goto err_vqs;
+   if (!names || !callbacks || !vqs) {
+   err = -ENOMEM;
+   goto out;
+   }
 
for (i = 0; i < num_vqs; i++) {
callbacks[i] = virtblk_done;
@@ -420,7 +414,7 @@ static int init_vq(struct virtio_blk *vblk)
/* Discover virtqueues and write information to configuration.  */
err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
if (err)
-   goto err_find_vqs;
+   goto out;
 
for (i = 0; i < num_vqs; i++) {
spin_lock_init(>vqs[i].lock);
@@ -428,16 +422,12 @@ static int init_vq(struct virtio_blk *vblk)
}
vblk->num_vqs = num_vqs;
 
- err_find_vqs:
+out:
kfree(vqs);
- err_vqs:
kfree(callbacks);
- err_callbacks:
kfree(names);
- err_names:
if (err)
kfree(vblk->vqs);
- out:
return err;
 }
 
-- 
2.7.4 (Apple Git-66)

[PATCH v3] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

From: Minfei Huang 

We do a lot of memory allocation in function init_vq, and don't handle
the allocation failure properly. Then this function will return 0,
although initialization fails due to lacking memory. At that moment,
kernel will panic in guest machine, if virtio is used to drive disk.

To fix this bug, we should take care of allocation failure, and return
correct value to let caller know what happen.

Tested-by: Chao Fan 
Signed-off-by: Minfei Huang 
Signed-off-by: Minfei Huang 
---
v2:
- Remove useless initialisation to NULL
v1:
- Refactor the patch to make code more readable
---
 drivers/block/virtio_blk.c | 26 --
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 42758b5..4ee78c0 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -394,22 +394,16 @@ static int init_vq(struct virtio_blk *vblk)
num_vqs = 1;
 
vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
-   if (!vblk->vqs) {
-   err = -ENOMEM;
-   goto out;
-   }
+   if (!vblk->vqs)
+   return -ENOMEM;
 
names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
-   if (!names)
-   goto err_names;
-
callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
-   if (!callbacks)
-   goto err_callbacks;
-
vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
-   if (!vqs)
-   goto err_vqs;
+   if (!names || !callbacks || !vqs) {
+   err = -ENOMEM;
+   goto out;
+   }
 
for (i = 0; i < num_vqs; i++) {
callbacks[i] = virtblk_done;
@@ -420,7 +414,7 @@ static int init_vq(struct virtio_blk *vblk)
/* Discover virtqueues and write information to configuration.  */
err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
if (err)
-   goto err_find_vqs;
+   goto out;
 
for (i = 0; i < num_vqs; i++) {
spin_lock_init(>vqs[i].lock);
@@ -428,16 +422,12 @@ static int init_vq(struct virtio_blk *vblk)
}
vblk->num_vqs = num_vqs;
 
- err_find_vqs:
+out:
kfree(vqs);
- err_vqs:
kfree(callbacks);
- err_callbacks:
kfree(names);
- err_names:
if (err)
kfree(vblk->vqs);
- out:
return err;
 }
 
-- 
2.7.4 (Apple Git-66)

Re: [PATCH v2] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

On 07/18/16 at 06:25P, Cornelia Huck wrote:
> On Tue, 19 Jul 2016 00:18:32 +0800
> Minfei Huang <mnfhu...@gmail.com> wrote:
> 
> > On 07/18/16 at 05:21P, Cornelia Huck wrote:
> > > On Mon, 18 Jul 2016 22:01:29 +0800
> > > Minfei Huang <mnfhu...@gmail.com> wrote:
> > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > > index 42758b5..d920512 100644
> > > > --- a/drivers/block/virtio_blk.c
> > > > +++ b/drivers/block/virtio_blk.c
> > > > @@ -381,9 +381,9 @@ static int init_vq(struct virtio_blk *vblk)
> > > >  {
> > > > int err = 0;
> > > > int i;
> > > > -   vq_callback_t **callbacks;
> > > > -   const char **names;
> > > > -   struct virtqueue **vqs;
> > > > +   vq_callback_t **callbacks = NULL;
> > > > +   const char **names = NULL;
> > > > +   struct virtqueue **vqs = NULL;
> > > 
> > > If you init the variables to NULL anyway...
> > 
> > Hi, Cornelia.
> > 
> > Thanks for reviewing this patch.
> > 
> > Seems there is no need to init these variables to NULL. I will remove
> > them laster.
> 
> Fine with me.
> 
> > 
> > > 
> > > > unsigned short num_vqs;
> > > > struct virtio_device *vdev = vblk->vdev;
> > > > 
> > > > @@ -394,22 +394,16 @@ static int init_vq(struct virtio_blk *vblk)
> > > > num_vqs = 1;
> > > > 
> > > 
> > > ...just do
> > > 
> > > err = -ENOMEM;
> > > 
> > > here and...
> > > 
> > > > vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
> > > > -   if (!vblk->vqs) {
> > > > -   err = -ENOMEM;
> > > > -   goto out;
> > > > -   }
> > > > +   if (!vblk->vqs)
> > > > +   return -ENOMEM;
> > > > 
> > > > names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
> > > > -   if (!names)
> > > > -   goto err_names;
> > > > -
> > > > callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
> > > > -   if (!callbacks)
> > > > -   goto err_callbacks;
> > > > -
> > > > vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
> > > > -   if (!vqs)
> > > > -   goto err_vqs;
> > > > +   if (!names || !callbacks || !vqs) {
> > > > +   err = -ENOMEM;
> > > > +   goto out;
> > > > +   }
> > > 
> > > ...you could use the
> > > 
> > > foo = kmalloc(...);
> > > if (!foo)
> > >   goto out;
> > > 
> > > sequence in any case. This avoids trying again and again if e.g. the
> > > names allocation already failed.
> > 
> > For this implementation, I have referred others which calls
> > vdev->config->find_vqs as well. Yes, this continues trying to allocate
> > memory, although memory allocation failed before.
> 
> It might not be the best idea, though; although it should hopefully be
> a not-so-common occurrence.

Yep, for that memont, there is enough memory to be allocated.

> 
> > 
> > > 
> > > Alternatively, you should be fine if you don't init the variables to
> > > NULL: The code is now either taking an early exit or setting all of the
> > > variables anyway.
> > > 
> > 
> > It's a big change if we refactor the helper ->find_vqs, since other
> > devices also call it.
> 
> Actually, I was referring to not initializing the variables to NULL in
> this function and keeping the rest of your changes: IOW, just what you
> suggested above :)

Ok. I will repost an update to fix it.

Thanks
Minfei

> 

Re: [PATCH v2] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

On 07/18/16 at 06:25P, Cornelia Huck wrote:
> On Tue, 19 Jul 2016 00:18:32 +0800
> Minfei Huang  wrote:
> 
> > On 07/18/16 at 05:21P, Cornelia Huck wrote:
> > > On Mon, 18 Jul 2016 22:01:29 +0800
> > > Minfei Huang  wrote:
> > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > > index 42758b5..d920512 100644
> > > > --- a/drivers/block/virtio_blk.c
> > > > +++ b/drivers/block/virtio_blk.c
> > > > @@ -381,9 +381,9 @@ static int init_vq(struct virtio_blk *vblk)
> > > >  {
> > > > int err = 0;
> > > > int i;
> > > > -   vq_callback_t **callbacks;
> > > > -   const char **names;
> > > > -   struct virtqueue **vqs;
> > > > +   vq_callback_t **callbacks = NULL;
> > > > +   const char **names = NULL;
> > > > +   struct virtqueue **vqs = NULL;
> > > 
> > > If you init the variables to NULL anyway...
> > 
> > Hi, Cornelia.
> > 
> > Thanks for reviewing this patch.
> > 
> > Seems there is no need to init these variables to NULL. I will remove
> > them laster.
> 
> Fine with me.
> 
> > 
> > > 
> > > > unsigned short num_vqs;
> > > > struct virtio_device *vdev = vblk->vdev;
> > > > 
> > > > @@ -394,22 +394,16 @@ static int init_vq(struct virtio_blk *vblk)
> > > > num_vqs = 1;
> > > > 
> > > 
> > > ...just do
> > > 
> > > err = -ENOMEM;
> > > 
> > > here and...
> > > 
> > > > vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
> > > > -   if (!vblk->vqs) {
> > > > -   err = -ENOMEM;
> > > > -   goto out;
> > > > -   }
> > > > +   if (!vblk->vqs)
> > > > +   return -ENOMEM;
> > > > 
> > > > names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
> > > > -   if (!names)
> > > > -   goto err_names;
> > > > -
> > > > callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
> > > > -   if (!callbacks)
> > > > -   goto err_callbacks;
> > > > -
> > > > vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
> > > > -   if (!vqs)
> > > > -   goto err_vqs;
> > > > +   if (!names || !callbacks || !vqs) {
> > > > +   err = -ENOMEM;
> > > > +   goto out;
> > > > +   }
> > > 
> > > ...you could use the
> > > 
> > > foo = kmalloc(...);
> > > if (!foo)
> > >   goto out;
> > > 
> > > sequence in any case. This avoids trying again and again if e.g. the
> > > names allocation already failed.
> > 
> > For this implementation, I have referred others which calls
> > vdev->config->find_vqs as well. Yes, this continues trying to allocate
> > memory, although memory allocation failed before.
> 
> It might not be the best idea, though; although it should hopefully be
> a not-so-common occurrence.

Yep, for that memont, there is enough memory to be allocated.

> 
> > 
> > > 
> > > Alternatively, you should be fine if you don't init the variables to
> > > NULL: The code is now either taking an early exit or setting all of the
> > > variables anyway.
> > > 
> > 
> > It's a big change if we refactor the helper ->find_vqs, since other
> > devices also call it.
> 
> Actually, I was referring to not initializing the variables to NULL in
> this function and keeping the rest of your changes: IOW, just what you
> suggested above :)

Ok. I will repost an update to fix it.

Thanks
Minfei

> 

Re: [PATCH v2] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

On 07/18/16 at 05:21P, Cornelia Huck wrote:
> On Mon, 18 Jul 2016 22:01:29 +0800
> Minfei Huang <mnfhu...@gmail.com> wrote:
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 42758b5..d920512 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -381,9 +381,9 @@ static int init_vq(struct virtio_blk *vblk)
> >  {
> > int err = 0;
> > int i;
> > -   vq_callback_t **callbacks;
> > -   const char **names;
> > -   struct virtqueue **vqs;
> > +   vq_callback_t **callbacks = NULL;
> > +   const char **names = NULL;
> > +   struct virtqueue **vqs = NULL;
> 
> If you init the variables to NULL anyway...

Hi, Cornelia.

Thanks for reviewing this patch.

Seems there is no need to init these variables to NULL. I will remove
them laster.

> 
> > unsigned short num_vqs;
> > struct virtio_device *vdev = vblk->vdev;
> > 
> > @@ -394,22 +394,16 @@ static int init_vq(struct virtio_blk *vblk)
> > num_vqs = 1;
> > 
> 
> ...just do
> 
> err = -ENOMEM;
> 
> here and...
> 
> > vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
> > -   if (!vblk->vqs) {
> > -   err = -ENOMEM;
> > -   goto out;
> > -   }
> > +   if (!vblk->vqs)
> > +   return -ENOMEM;
> > 
> > names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
> > -   if (!names)
> > -   goto err_names;
> > -
> > callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
> > -   if (!callbacks)
> > -   goto err_callbacks;
> > -
> > vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
> > -   if (!vqs)
> > -   goto err_vqs;
> > +   if (!names || !callbacks || !vqs) {
> > +   err = -ENOMEM;
> > +   goto out;
> > +   }
> 
> ...you could use the
> 
> foo = kmalloc(...);
> if (!foo)
>   goto out;
> 
> sequence in any case. This avoids trying again and again if e.g. the
> names allocation already failed.

For this implementation, I have referred others which calls
vdev->config->find_vqs as well. Yes, this continues trying to allocate
memory, although memory allocation failed before.

> 
> Alternatively, you should be fine if you don't init the variables to
> NULL: The code is now either taking an early exit or setting all of the
> variables anyway.
> 

It's a big change if we refactor the helper ->find_vqs, since other
devices also call it.

Thanks
Minfei

Re: [PATCH v2] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

On 07/18/16 at 05:21P, Cornelia Huck wrote:
> On Mon, 18 Jul 2016 22:01:29 +0800
> Minfei Huang  wrote:
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 42758b5..d920512 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -381,9 +381,9 @@ static int init_vq(struct virtio_blk *vblk)
> >  {
> > int err = 0;
> > int i;
> > -   vq_callback_t **callbacks;
> > -   const char **names;
> > -   struct virtqueue **vqs;
> > +   vq_callback_t **callbacks = NULL;
> > +   const char **names = NULL;
> > +   struct virtqueue **vqs = NULL;
> 
> If you init the variables to NULL anyway...

Hi, Cornelia.

Thanks for reviewing this patch.

Seems there is no need to init these variables to NULL. I will remove
them laster.

> 
> > unsigned short num_vqs;
> > struct virtio_device *vdev = vblk->vdev;
> > 
> > @@ -394,22 +394,16 @@ static int init_vq(struct virtio_blk *vblk)
> > num_vqs = 1;
> > 
> 
> ...just do
> 
> err = -ENOMEM;
> 
> here and...
> 
> > vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
> > -   if (!vblk->vqs) {
> > -   err = -ENOMEM;
> > -   goto out;
> > -   }
> > +   if (!vblk->vqs)
> > +   return -ENOMEM;
> > 
> > names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
> > -   if (!names)
> > -   goto err_names;
> > -
> > callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
> > -   if (!callbacks)
> > -   goto err_callbacks;
> > -
> > vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
> > -   if (!vqs)
> > -   goto err_vqs;
> > +   if (!names || !callbacks || !vqs) {
> > +   err = -ENOMEM;
> > +   goto out;
> > +   }
> 
> ...you could use the
> 
> foo = kmalloc(...);
> if (!foo)
>   goto out;
> 
> sequence in any case. This avoids trying again and again if e.g. the
> names allocation already failed.

For this implementation, I have referred others which calls
vdev->config->find_vqs as well. Yes, this continues trying to allocate
memory, although memory allocation failed before.

> 
> Alternatively, you should be fine if you don't init the variables to
> NULL: The code is now either taking an early exit or setting all of the
> variables anyway.
> 

It's a big change if we refactor the helper ->find_vqs, since other
devices also call it.

Thanks
Minfei

[PATCH v2] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

We do a lot of memory allocation in function init_vq, and don't handle
the allocation failure properly. Then this function will return 0,
although initialization fails due to lacking memory. At that moment,
kernel will panic in guest machine, if virtio is used to drive disk.

To fix this bug, we should take care of allocation failure, and return
correct value to let caller know what happen.

Tested-by: Chao Fan <fanc.f...@cn.fujitsu.com>
Signed-off-by: Minfei Huang <minfei@alibaba-inc.com>
Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
v1:
- Refactor the patch to make code more readable
---
 drivers/block/virtio_blk.c | 32 +++-
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 42758b5..d920512 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -381,9 +381,9 @@ static int init_vq(struct virtio_blk *vblk)
 {
int err = 0;
int i;
-   vq_callback_t **callbacks;
-   const char **names;
-   struct virtqueue **vqs;
+   vq_callback_t **callbacks = NULL;
+   const char **names = NULL;
+   struct virtqueue **vqs = NULL;
unsigned short num_vqs;
struct virtio_device *vdev = vblk->vdev;
 
@@ -394,22 +394,16 @@ static int init_vq(struct virtio_blk *vblk)
num_vqs = 1;
 
vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
-   if (!vblk->vqs) {
-   err = -ENOMEM;
-   goto out;
-   }
+   if (!vblk->vqs)
+   return -ENOMEM;
 
names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
-   if (!names)
-   goto err_names;
-
callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
-   if (!callbacks)
-   goto err_callbacks;
-
vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
-   if (!vqs)
-   goto err_vqs;
+   if (!names || !callbacks || !vqs) {
+   err = -ENOMEM;
+   goto out;
+   }
 
for (i = 0; i < num_vqs; i++) {
callbacks[i] = virtblk_done;
@@ -420,7 +414,7 @@ static int init_vq(struct virtio_blk *vblk)
/* Discover virtqueues and write information to configuration.  */
err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
if (err)
-   goto err_find_vqs;
+   goto out;
 
for (i = 0; i < num_vqs; i++) {
spin_lock_init(>vqs[i].lock);
@@ -428,16 +422,12 @@ static int init_vq(struct virtio_blk *vblk)
}
vblk->num_vqs = num_vqs;
 
- err_find_vqs:
+out:
kfree(vqs);
- err_vqs:
kfree(callbacks);
- err_callbacks:
kfree(names);
- err_names:
if (err)
kfree(vblk->vqs);
- out:
return err;
 }
 
-- 
2.7.4 (Apple Git-66)

[PATCH v2] virtio_blk: Fix a slient kernel panic

[Minfei Huang]

We do a lot of memory allocation in function init_vq, and don't handle
the allocation failure properly. Then this function will return 0,
although initialization fails due to lacking memory. At that moment,
kernel will panic in guest machine, if virtio is used to drive disk.

To fix this bug, we should take care of allocation failure, and return
correct value to let caller know what happen.

Tested-by: Chao Fan 
Signed-off-by: Minfei Huang 
Signed-off-by: Minfei Huang 
---
v1:
- Refactor the patch to make code more readable
---
 drivers/block/virtio_blk.c | 32 +++-
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 42758b5..d920512 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -381,9 +381,9 @@ static int init_vq(struct virtio_blk *vblk)
 {
int err = 0;
int i;
-   vq_callback_t **callbacks;
-   const char **names;
-   struct virtqueue **vqs;
+   vq_callback_t **callbacks = NULL;
+   const char **names = NULL;
+   struct virtqueue **vqs = NULL;
unsigned short num_vqs;
struct virtio_device *vdev = vblk->vdev;
 
@@ -394,22 +394,16 @@ static int init_vq(struct virtio_blk *vblk)
num_vqs = 1;
 
vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
-   if (!vblk->vqs) {
-   err = -ENOMEM;
-   goto out;
-   }
+   if (!vblk->vqs)
+   return -ENOMEM;
 
names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
-   if (!names)
-   goto err_names;
-
callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
-   if (!callbacks)
-   goto err_callbacks;
-
vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
-   if (!vqs)
-   goto err_vqs;
+   if (!names || !callbacks || !vqs) {
+   err = -ENOMEM;
+   goto out;
+   }
 
for (i = 0; i < num_vqs; i++) {
callbacks[i] = virtblk_done;
@@ -420,7 +414,7 @@ static int init_vq(struct virtio_blk *vblk)
/* Discover virtqueues and write information to configuration.  */
err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
if (err)
-   goto err_find_vqs;
+   goto out;
 
for (i = 0; i < num_vqs; i++) {
spin_lock_init(>vqs[i].lock);
@@ -428,16 +422,12 @@ static int init_vq(struct virtio_blk *vblk)
}
vblk->num_vqs = num_vqs;
 
- err_find_vqs:
+out:
kfree(vqs);
- err_vqs:
kfree(callbacks);
- err_callbacks:
kfree(names);
- err_names:
if (err)
kfree(vblk->vqs);
- out:
return err;
 }
 
-- 
2.7.4 (Apple Git-66)

Re: [PATCH] virtio: Return correct errno for function init_vq's failure

[Minfei Huang]

On 07/06/16 at 11:18P, Cornelia Huck wrote:
> On Mon, 27 Jun 2016 10:09:18 +0800
> Minfei Huang <mngh...@gmail.com> wrote:
> 
> > The error number -ENOENT or 0 will be returned, if we can not allocate
> > more memory in function init_vq. If host can support multiple virtual
> > queues, and we fails to allocate necessary memory structures for vq,
> > kernel may crash due to incorrect returning.
> > 
> > To fix it, kernel will return correct value in init_vq.
> The error handling in this function looks horrible.
> 
> When mq was introduced, init_vq started mixing up several things:
> - The mq feature is not available - which is not an error, and
> therefore should not have any influence on the return code.
> - One of the several memory allocations failed - only ->vqs gets
> special treatment, however.
> - The ->find_vqs callback failed.

Yep. And without this patch, it is silent for boot failure. I think it
makes sense to let user notify about this failure.

> 
> Your patch fixes the code, but it is still very convoluted due to the
> temporary arrays.
> 
> May it be worthwile to introduce a helper for setting up the virtqueues
> where all virtqueues are essentially the same and just get a
> consecutive number? Michael?
> 

Hmm, How about refactor this function to make it more readable, since we
do a lot of work in it.

I will post an update to refactor this function.

Thanks
Minfei

Re: [PATCH] virtio: Return correct errno for function init_vq's failure

[Minfei Huang]

On 07/06/16 at 11:18P, Cornelia Huck wrote:
> On Mon, 27 Jun 2016 10:09:18 +0800
> Minfei Huang  wrote:
> 
> > The error number -ENOENT or 0 will be returned, if we can not allocate
> > more memory in function init_vq. If host can support multiple virtual
> > queues, and we fails to allocate necessary memory structures for vq,
> > kernel may crash due to incorrect returning.
> > 
> > To fix it, kernel will return correct value in init_vq.
> The error handling in this function looks horrible.
> 
> When mq was introduced, init_vq started mixing up several things:
> - The mq feature is not available - which is not an error, and
> therefore should not have any influence on the return code.
> - One of the several memory allocations failed - only ->vqs gets
> special treatment, however.
> - The ->find_vqs callback failed.

Yep. And without this patch, it is silent for boot failure. I think it
makes sense to let user notify about this failure.

> 
> Your patch fixes the code, but it is still very convoluted due to the
> temporary arrays.
> 
> May it be worthwile to introduce a helper for setting up the virtqueues
> where all virtqueues are essentially the same and just get a
> consecutive number? Michael?
> 

Hmm, How about refactor this function to make it more readable, since we
do a lot of work in it.

I will post an update to refactor this function.

Thanks
Minfei

Re: [PATCH RESENT] dm: Check kthread_run's return value

[Minfei Huang]

On 07/06/16 at 09:31P, Mike Snitzer wrote:
> On Wed, Jul 06 2016 at  9:27am -0400,
> Minfei Huang <mngh...@gmail.com> wrote:
> 
> > On 07/06/16 at 09:16P, Mike Snitzer wrote:
> > > On Mon, Jul 04 2016 at 11:25am -0400,
> > > Minfei Huang <mngh...@gmail.com> wrote:
> > > 
> > > > kthread function is used to process kthread_work. And there is no return
> > > > value checking during create this thread. Add this checking to fix this
> > > > issue.
> > > > 
> > > > Signed-off-by: Minfei Huang <mngh...@gmail.com>
> > > This needed rebasing against linux-dm.git's 'for-next'.  I've now staged
> > > this fix for 4.8 inclusion, see:
> > > https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next=7193a9defcab6f3d3f1eb64c68bad7534e5a39ad
> > 
> > Seems that we should fix it in stable as well.
> 
> Given the code movement it isn't easy to do (by simply adding a stable@
> cc).  I've not seen a single report of an ignored kthread_run() failure
> for multipath using .request_fn interface.

Yep, this bug will be trigged in very restrict condition, also dm is
installed in boot time when there are a lot of memory avaiable.

Thanks
Minfei

Re: [PATCH RESENT] dm: Check kthread_run's return value

[Minfei Huang]

On 07/06/16 at 09:31P, Mike Snitzer wrote:
> On Wed, Jul 06 2016 at  9:27am -0400,
> Minfei Huang  wrote:
> 
> > On 07/06/16 at 09:16P, Mike Snitzer wrote:
> > > On Mon, Jul 04 2016 at 11:25am -0400,
> > > Minfei Huang  wrote:
> > > 
> > > > kthread function is used to process kthread_work. And there is no return
> > > > value checking during create this thread. Add this checking to fix this
> > > > issue.
> > > > 
> > > > Signed-off-by: Minfei Huang 
> > > This needed rebasing against linux-dm.git's 'for-next'.  I've now staged
> > > this fix for 4.8 inclusion, see:
> > > https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next=7193a9defcab6f3d3f1eb64c68bad7534e5a39ad
> > 
> > Seems that we should fix it in stable as well.
> 
> Given the code movement it isn't easy to do (by simply adding a stable@
> cc).  I've not seen a single report of an ignored kthread_run() failure
> for multipath using .request_fn interface.

Yep, this bug will be trigged in very restrict condition, also dm is
installed in boot time when there are a lot of memory avaiable.

Thanks
Minfei

Re: [PATCH RESENT] dm: Check kthread_run's return value

[Minfei Huang]

On 07/06/16 at 09:16P, Mike Snitzer wrote:
> On Mon, Jul 04 2016 at 11:25am -0400,
> Minfei Huang <mngh...@gmail.com> wrote:
> 
> > kthread function is used to process kthread_work. And there is no return
> > value checking during create this thread. Add this checking to fix this
> > issue.
> > 
> > Signed-off-by: Minfei Huang <mngh...@gmail.com>
> This needed rebasing against linux-dm.git's 'for-next'.  I've now staged
> this fix for 4.8 inclusion, see:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next=7193a9defcab6f3d3f1eb64c68bad7534e5a39ad

Seems that we should fix it in stable as well.

Thanks
Minfei

Re: [PATCH RESENT] dm: Check kthread_run's return value

[Minfei Huang]

On 07/06/16 at 09:16P, Mike Snitzer wrote:
> On Mon, Jul 04 2016 at 11:25am -0400,
> Minfei Huang  wrote:
> 
> > kthread function is used to process kthread_work. And there is no return
> > value checking during create this thread. Add this checking to fix this
> > issue.
> > 
> > Signed-off-by: Minfei Huang 
> This needed rebasing against linux-dm.git's 'for-next'.  I've now staged
> this fix for 4.8 inclusion, see:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next=7193a9defcab6f3d3f1eb64c68bad7534e5a39ad

Seems that we should fix it in stable as well.

Thanks
Minfei

[PATCH RESENT] dm: Check kthread_run's return value

[Minfei Huang]

kthread function is used to process kthread_work. And there is no return
value checking during create this thread. Add this checking to fix this
issue.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 drivers/md/dm.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 1b2f962..d68b9d2 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2654,12 +2654,15 @@ struct queue_limits *dm_get_queue_limits(struct 
mapped_device *md)
 }
 EXPORT_SYMBOL_GPL(dm_get_queue_limits);
 
-static void dm_old_init_rq_based_worker_thread(struct mapped_device *md)
+static int dm_old_init_rq_based_worker_thread(struct mapped_device *md)
 {
/* Initialize the request-based DM worker thread */
init_kthread_worker(>kworker);
md->kworker_task = kthread_run(kthread_worker_fn, >kworker,
   "kdmwork-%s", dm_device_name(md));
+   if (IS_ERR(md->kworker_task))
+   return -ENOMEM;
+   return 0;
 }
 
 /*
@@ -2667,6 +2670,8 @@ static void dm_old_init_rq_based_worker_thread(struct 
mapped_device *md)
  */
 static int dm_old_init_request_queue(struct mapped_device *md)
 {
+   int ret;
+
/* Fully initialize the queue */
if (!blk_init_allocated_queue(md->queue, dm_request_fn, NULL))
return -EINVAL;
@@ -2678,7 +2683,9 @@ static int dm_old_init_request_queue(struct mapped_device 
*md)
blk_queue_softirq_done(md->queue, dm_softirq_done);
blk_queue_prep_rq(md->queue, dm_old_prep_fn);
 
-   dm_old_init_rq_based_worker_thread(md);
+   ret = dm_old_init_rq_based_worker_thread(md);
+   if (ret < 0)
+   return ret;
 
elv_register_queue(md->queue);
 
-- 
2.6.3

[PATCH RESENT] dm: Check kthread_run's return value

[Minfei Huang]

kthread function is used to process kthread_work. And there is no return
value checking during create this thread. Add this checking to fix this
issue.

Signed-off-by: Minfei Huang 
---
 drivers/md/dm.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 1b2f962..d68b9d2 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2654,12 +2654,15 @@ struct queue_limits *dm_get_queue_limits(struct 
mapped_device *md)
 }
 EXPORT_SYMBOL_GPL(dm_get_queue_limits);
 
-static void dm_old_init_rq_based_worker_thread(struct mapped_device *md)
+static int dm_old_init_rq_based_worker_thread(struct mapped_device *md)
 {
/* Initialize the request-based DM worker thread */
init_kthread_worker(>kworker);
md->kworker_task = kthread_run(kthread_worker_fn, >kworker,
   "kdmwork-%s", dm_device_name(md));
+   if (IS_ERR(md->kworker_task))
+   return -ENOMEM;
+   return 0;
 }
 
 /*
@@ -2667,6 +2670,8 @@ static void dm_old_init_rq_based_worker_thread(struct 
mapped_device *md)
  */
 static int dm_old_init_request_queue(struct mapped_device *md)
 {
+   int ret;
+
/* Fully initialize the queue */
if (!blk_init_allocated_queue(md->queue, dm_request_fn, NULL))
return -EINVAL;
@@ -2678,7 +2683,9 @@ static int dm_old_init_request_queue(struct mapped_device 
*md)
blk_queue_softirq_done(md->queue, dm_softirq_done);
blk_queue_prep_rq(md->queue, dm_old_prep_fn);
 
-   dm_old_init_rq_based_worker_thread(md);
+   ret = dm_old_init_rq_based_worker_thread(md);
+   if (ret < 0)
+   return ret;
 
elv_register_queue(md->queue);
 
-- 
2.6.3

[PATCH] virtio: Return correct errno for function init_vq's failure

[Minfei Huang]

The error number -ENOENT or 0 will be returned, if we can not allocate
more memory in function init_vq. If host can support multiple virtual
queues, and we fails to allocate necessary memory structures for vq,
kernel may crash due to incorrect returning.

To fix it, kernel will return correct value in init_vq.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
Signed-off-by: Minfei Huang <minfei@alibaba-inc.com>
---
 drivers/block/virtio_blk.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 42758b5..40ecb2b 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -393,11 +393,10 @@ static int init_vq(struct virtio_blk *vblk)
if (err)
num_vqs = 1;
 
+   err = -ENOMEM;
vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
-   if (!vblk->vqs) {
-   err = -ENOMEM;
+   if (!vblk->vqs)
goto out;
-   }
 
names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
if (!names)
-- 
2.7.4 (Apple Git-66)

[PATCH] virtio: Return correct errno for function init_vq's failure

[Minfei Huang]

The error number -ENOENT or 0 will be returned, if we can not allocate
more memory in function init_vq. If host can support multiple virtual
queues, and we fails to allocate necessary memory structures for vq,
kernel may crash due to incorrect returning.

To fix it, kernel will return correct value in init_vq.

Signed-off-by: Minfei Huang 
Signed-off-by: Minfei Huang 
---
 drivers/block/virtio_blk.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 42758b5..40ecb2b 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -393,11 +393,10 @@ static int init_vq(struct virtio_blk *vblk)
if (err)
num_vqs = 1;
 
+   err = -ENOMEM;
vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
-   if (!vblk->vqs) {
-   err = -ENOMEM;
+   if (!vblk->vqs)
goto out;
-   }
 
names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
if (!names)
-- 
2.7.4 (Apple Git-66)

Re: [PATCH] aio: Cleanup unnecessary test for nr_pages

[Minfei Huang]

On 06/20/16 at 03:17P, Al Viro wrote:
> On Mon, Jun 20, 2016 at 10:05:45PM +0800, Minfei Huang wrote:
> > Ping. Any comment is appreciate.
> > 
> > Thanks
> > Minfei
> > 
> > On 06/13/16 at 12:33P, Minfei Huang wrote:
> > > The variable nr_pages is always more than 1, because the size of
> > > structure aio_ring is bigger than 0. So remove unnecessary test for
> > > nr_page.
> 
> What this test really checks is that the value we'd put into nr_pages
> (PFN_UP(size)) is not greater than 2^31.  Whether it's redundant or not
> is a separate question - it very well might be, due to the code in
> ioctx_alloc() that caps nr_events, but that needs a proof.  In any case,
> the reasons you are offering in commit message are wrong - it's about
> size being too _large_, not too small.

Hmm. But there is a following test in function ioctx_alloc which is used
to limit the max of nr_events.

 713 /* Prevent overflows */
 714 if (nr_events > (0x1000U / sizeof(struct io_event))) {
 715 pr_debug("ENOMEM: nr_events too high\n");
 716 return ERR_PTR(-EINVAL);
 717 }

So according to this test, we can make sure that nr_pages(PFN_UP(size))
cann't be greater than 2^31, and max to 2^28.

Thanks
Minfei

Re: [PATCH] aio: Cleanup unnecessary test for nr_pages

[Minfei Huang]

On 06/20/16 at 03:17P, Al Viro wrote:
> On Mon, Jun 20, 2016 at 10:05:45PM +0800, Minfei Huang wrote:
> > Ping. Any comment is appreciate.
> > 
> > Thanks
> > Minfei
> > 
> > On 06/13/16 at 12:33P, Minfei Huang wrote:
> > > The variable nr_pages is always more than 1, because the size of
> > > structure aio_ring is bigger than 0. So remove unnecessary test for
> > > nr_page.
> 
> What this test really checks is that the value we'd put into nr_pages
> (PFN_UP(size)) is not greater than 2^31.  Whether it's redundant or not
> is a separate question - it very well might be, due to the code in
> ioctx_alloc() that caps nr_events, but that needs a proof.  In any case,
> the reasons you are offering in commit message are wrong - it's about
> size being too _large_, not too small.

Hmm. But there is a following test in function ioctx_alloc which is used
to limit the max of nr_events.

 713 /* Prevent overflows */
 714 if (nr_events > (0x1000U / sizeof(struct io_event))) {
 715 pr_debug("ENOMEM: nr_events too high\n");
 716 return ERR_PTR(-EINVAL);
 717 }

So according to this test, we can make sure that nr_pages(PFN_UP(size))
cann't be greater than 2^31, and max to 2^28.

Thanks
Minfei

[PATCH] dm: Check kthread_run's return value

[Minfei Huang]

kthread function is used to process kthread_work. And there is no return
value checking during create this thread. Add this checking to fix this
issue.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
Signed-off-by: Minfei Huang <minfei@alibaba-inc.com>
---
 drivers/md/dm.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 1b2f962..d68b9d2 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2654,12 +2654,15 @@ struct queue_limits *dm_get_queue_limits(struct 
mapped_device *md)
 }
 EXPORT_SYMBOL_GPL(dm_get_queue_limits);
 
-static void dm_old_init_rq_based_worker_thread(struct mapped_device *md)
+static int dm_old_init_rq_based_worker_thread(struct mapped_device *md)
 {
/* Initialize the request-based DM worker thread */
init_kthread_worker(>kworker);
md->kworker_task = kthread_run(kthread_worker_fn, >kworker,
   "kdmwork-%s", dm_device_name(md));
+   if (IS_ERR(md->kworker_task))
+   return -ENOMEM;
+   return 0;
 }
 
 /*
@@ -2667,6 +2670,8 @@ static void dm_old_init_rq_based_worker_thread(struct 
mapped_device *md)
  */
 static int dm_old_init_request_queue(struct mapped_device *md)
 {
+   int ret;
+
/* Fully initialize the queue */
if (!blk_init_allocated_queue(md->queue, dm_request_fn, NULL))
return -EINVAL;
@@ -2678,7 +2683,9 @@ static int dm_old_init_request_queue(struct mapped_device 
*md)
blk_queue_softirq_done(md->queue, dm_softirq_done);
blk_queue_prep_rq(md->queue, dm_old_prep_fn);
 
-   dm_old_init_rq_based_worker_thread(md);
+   ret = dm_old_init_rq_based_worker_thread(md);
+   if (ret < 0)
+   return ret;
 
elv_register_queue(md->queue);
 
-- 
2.7.4 (Apple Git-66)

[PATCH] dm: Check kthread_run's return value

[Minfei Huang]

kthread function is used to process kthread_work. And there is no return
value checking during create this thread. Add this checking to fix this
issue.

Signed-off-by: Minfei Huang 
Signed-off-by: Minfei Huang 
---
 drivers/md/dm.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 1b2f962..d68b9d2 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2654,12 +2654,15 @@ struct queue_limits *dm_get_queue_limits(struct 
mapped_device *md)
 }
 EXPORT_SYMBOL_GPL(dm_get_queue_limits);
 
-static void dm_old_init_rq_based_worker_thread(struct mapped_device *md)
+static int dm_old_init_rq_based_worker_thread(struct mapped_device *md)
 {
/* Initialize the request-based DM worker thread */
init_kthread_worker(>kworker);
md->kworker_task = kthread_run(kthread_worker_fn, >kworker,
   "kdmwork-%s", dm_device_name(md));
+   if (IS_ERR(md->kworker_task))
+   return -ENOMEM;
+   return 0;
 }
 
 /*
@@ -2667,6 +2670,8 @@ static void dm_old_init_rq_based_worker_thread(struct 
mapped_device *md)
  */
 static int dm_old_init_request_queue(struct mapped_device *md)
 {
+   int ret;
+
/* Fully initialize the queue */
if (!blk_init_allocated_queue(md->queue, dm_request_fn, NULL))
return -EINVAL;
@@ -2678,7 +2683,9 @@ static int dm_old_init_request_queue(struct mapped_device 
*md)
blk_queue_softirq_done(md->queue, dm_softirq_done);
blk_queue_prep_rq(md->queue, dm_old_prep_fn);
 
-   dm_old_init_rq_based_worker_thread(md);
+   ret = dm_old_init_rq_based_worker_thread(md);
+   if (ret < 0)
+   return ret;
 
elv_register_queue(md->queue);
 
-- 
2.7.4 (Apple Git-66)

Re: [PATCH] aio: Cleanup unnecessary test for nr_pages

[Minfei Huang]

Ping. Any comment is appreciate.

Thanks
Minfei

On 06/13/16 at 12:33P, Minfei Huang wrote:
> The variable nr_pages is always more than 1, because the size of
> structure aio_ring is bigger than 0. So remove unnecessary test for
> nr_page.
> 
> Signed-off-by: Minfei Huang <mngh...@gmail.com>
> ---
>  fs/aio.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index fb8e45b..ec05137 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -450,8 +450,6 @@ static int aio_setup_ring(struct kioctx *ctx)
>   size += sizeof(struct io_event) * nr_events;
>  
>   nr_pages = PFN_UP(size);
> - if (nr_pages < 0)
> - return -EINVAL;
>  
>   file = aio_private_file(ctx, nr_pages);
>   if (IS_ERR(file)) {
> -- 
> 2.7.4 (Apple Git-66)
> 

Re: [PATCH] aio: Cleanup unnecessary test for nr_pages

[Minfei Huang]

Ping. Any comment is appreciate.

Thanks
Minfei

On 06/13/16 at 12:33P, Minfei Huang wrote:
> The variable nr_pages is always more than 1, because the size of
> structure aio_ring is bigger than 0. So remove unnecessary test for
> nr_page.
> 
> Signed-off-by: Minfei Huang 
> ---
>  fs/aio.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index fb8e45b..ec05137 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -450,8 +450,6 @@ static int aio_setup_ring(struct kioctx *ctx)
>   size += sizeof(struct io_event) * nr_events;
>  
>   nr_pages = PFN_UP(size);
> - if (nr_pages < 0)
> - return -EINVAL;
>  
>   file = aio_private_file(ctx, nr_pages);
>   if (IS_ERR(file)) {
> -- 
> 2.7.4 (Apple Git-66)
> 

[PATCH] aio: Cleanup unnecessary test for nr_pages

[Minfei Huang]

The variable nr_pages is always more than 1, because the size of
structure aio_ring is bigger than 0. So remove unnecessary test for
nr_page.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 fs/aio.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index fb8e45b..ec05137 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -450,8 +450,6 @@ static int aio_setup_ring(struct kioctx *ctx)
size += sizeof(struct io_event) * nr_events;
 
nr_pages = PFN_UP(size);
-   if (nr_pages < 0)
-   return -EINVAL;
 
file = aio_private_file(ctx, nr_pages);
if (IS_ERR(file)) {
-- 
2.7.4 (Apple Git-66)

[PATCH] aio: Cleanup unnecessary test for nr_pages

[Minfei Huang]

The variable nr_pages is always more than 1, because the size of
structure aio_ring is bigger than 0. So remove unnecessary test for
nr_page.

Signed-off-by: Minfei Huang 
---
 fs/aio.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index fb8e45b..ec05137 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -450,8 +450,6 @@ static int aio_setup_ring(struct kioctx *ctx)
size += sizeof(struct io_event) * nr_events;
 
nr_pages = PFN_UP(size);
-   if (nr_pages < 0)
-   return -EINVAL;
 
file = aio_private_file(ctx, nr_pages);
if (IS_ERR(file)) {
-- 
2.7.4 (Apple Git-66)

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Minfei Huang]

On 06/09/16 at 01:28P, Borislav Petkov wrote:
> On Thu, Jun 09, 2016 at 01:21:18PM +0200, Paolo Bonzini wrote:
> > This is basically implementing a seqcount.  It needs two barriers and,
> 
> Why does it need the two barriers? More details please.

Hi, Borislav.

It's a seqcount-like. We should confirm that reading flags occurs
between two reading version. To make complier do not reorder, two
barriers are necessary.

Thanks
Minfei

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Minfei Huang]

On 06/09/16 at 01:28P, Borislav Petkov wrote:
> On Thu, Jun 09, 2016 at 01:21:18PM +0200, Paolo Bonzini wrote:
> > This is basically implementing a seqcount.  It needs two barriers and,
> 
> Why does it need the two barriers? More details please.

Hi, Borislav.

It's a seqcount-like. We should confirm that reading flags occurs
between two reading version. To make complier do not reorder, two
barriers are necessary.

Thanks
Minfei

Re: [PATCH] pvclock: introduce seqcount-like API

[Minfei Huang]

On 06/09/16 at 01:23P, Paolo Bonzini wrote:
> The version field in struct pvclock_vcpu_time_info basically implements
> a seqcount.  Wrap it with the usual read_begin and read_retry functions,
> and use these APIs instead of peppering the code with smp_rmb()s.
> While at it, change it to the more pedantically correct virt_rmb().
> 
> With this change, __pvclock_read_cycles can be simplified noticeably.

Hi, Paolo.

Thanks for accepting my patches in your repo.

> 
> Signed-off-by: Paolo Bonzini 
> ---
>  arch/x86/entry/vdso/vclock_gettime.c | 25 +--
>  arch/x86/include/asm/pvclock.h   | 39 
> +---
>  arch/x86/kernel/pvclock.c| 17 ++--
>  3 files changed, 34 insertions(+), 47 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> index 7c1c89598688..0ee92db1e9f3 100644
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
> @@ -25,6 +25,24 @@ void pvclock_resume(void);
>  
>  void pvclock_touch_watchdogs(void);
>  
> +static __always_inline
> +unsigned pvclock_read_begin(const struct pvclock_vcpu_time_info *src)

It's better to use type unsigned int, instead of unsigned which is
complained by script checkpatch.

Thanks
Minfei

> +{
> + unsigned version = src->version & ~1;

Ditto.

> + /* Make sure that the version is read before the data. */
> + virt_rmb();
> + return version;
> +}
> +
> +static __always_inline
> +bool pvclock_read_retry(const struct pvclock_vcpu_time_info *src,
> + unsigned version)

Ditto.

Thanks
Minfei

Re: [PATCH] pvclock: introduce seqcount-like API

[Minfei Huang]

On 06/09/16 at 01:23P, Paolo Bonzini wrote:
> The version field in struct pvclock_vcpu_time_info basically implements
> a seqcount.  Wrap it with the usual read_begin and read_retry functions,
> and use these APIs instead of peppering the code with smp_rmb()s.
> While at it, change it to the more pedantically correct virt_rmb().
> 
> With this change, __pvclock_read_cycles can be simplified noticeably.

Hi, Paolo.

Thanks for accepting my patches in your repo.

> 
> Signed-off-by: Paolo Bonzini 
> ---
>  arch/x86/entry/vdso/vclock_gettime.c | 25 +--
>  arch/x86/include/asm/pvclock.h   | 39 
> +---
>  arch/x86/kernel/pvclock.c| 17 ++--
>  3 files changed, 34 insertions(+), 47 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> index 7c1c89598688..0ee92db1e9f3 100644
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
> @@ -25,6 +25,24 @@ void pvclock_resume(void);
>  
>  void pvclock_touch_watchdogs(void);
>  
> +static __always_inline
> +unsigned pvclock_read_begin(const struct pvclock_vcpu_time_info *src)

It's better to use type unsigned int, instead of unsigned which is
complained by script checkpatch.

Thanks
Minfei

> +{
> + unsigned version = src->version & ~1;

Ditto.

> + /* Make sure that the version is read before the data. */
> + virt_rmb();
> + return version;
> +}
> +
> +static __always_inline
> +bool pvclock_read_retry(const struct pvclock_vcpu_time_info *src,
> + unsigned version)

Ditto.

Thanks
Minfei

Re: [PATCH] x86:pvclock: add missing barriers

[Minfei Huang]

On 06/08/16 at 09:45P, Borislav Petkov wrote:
> On Wed, Jun 08, 2016 at 09:11:39PM +0300, Roman Kagan wrote:
> > Gradual removal of excessive barriers in pvclock reading functions
> > (commits 502dfeff239e8313bfbe906ca0a1a6827ac8481b,
> > a3eb97bd80134ba07864ca00747466c02118aca1) ended up removing too much:
> > although rdtsc is now orderd WRT other loads, there's no protection
> > against the compiler reordering the loads of ->version with the loads of
> > other fields.
> > 
> > E.g. on my system gcc-5.3.1 generates code which loads ->system_time and
> > ->flags outside of the ->version test loop.
> > 
> > (Re)introduce the compiler barriers around accesses to the contents of
> > pvclock.  While at this, make the function a bit more compact by
> > removing unnecessary local variables.
> > 
> > Signed-off-by: Roman Kagan 
> > Cc: Thomas Gleixner 
> > Cc: Ingo Molnar 
> > Cc: "H. Peter Anvin" 
> > Cc: x...@kernel.org
> > Cc: Andy Lutomirski 
> > Cc: Borislav Petkov 
> > Cc: Paolo Bonzini 
> > Cc: sta...@vger.kernel.org
> > ---
> >  arch/x86/include/asm/pvclock.h | 17 +
> >  1 file changed, 5 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> > index fdcc040..65c4de2 100644
> > --- a/arch/x86/include/asm/pvclock.h
> > +++ b/arch/x86/include/asm/pvclock.h
> > @@ -80,18 +80,11 @@ static __always_inline
> >  unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
> >cycle_t *cycles, u8 *flags)
> >  {
> > -   unsigned version;
> > -   cycle_t ret, offset;
> > -   u8 ret_flags;
> > -
> > -   version = src->version;
> > -
> > -   offset = pvclock_get_nsec_offset(src);
> > -   ret = src->system_time + offset;
> > -   ret_flags = src->flags;
> > -
> > -   *cycles = ret;
> > -   *flags = ret_flags;
> > +   unsigned version = src->version;
> > +   barrier();
> > +   *cycles = src->system_time + pvclock_get_nsec_offset(src);
> > +   *flags = src->flags;
> > +   barrier();
> > return version;
> 
> I have a similar patchset in my mbox starting here:
> 
> https://lkml.kernel.org/r/1464329832-4638-1-git-send-email-mngh...@gmail.com
> 
> Care to take a look?

Hi, Boris.

I had a vocation last several days, and sorry for replying it
later.

As Roman said in this thread, both his and mine are similar way
to fix this inconsistent issue, and I have a wrapper function
smp_rmb in my patch which the root function is barriers in x86
as well.


Thanks
Minfei

Re: [PATCH] x86:pvclock: add missing barriers

[Minfei Huang]

On 06/08/16 at 09:45P, Borislav Petkov wrote:
> On Wed, Jun 08, 2016 at 09:11:39PM +0300, Roman Kagan wrote:
> > Gradual removal of excessive barriers in pvclock reading functions
> > (commits 502dfeff239e8313bfbe906ca0a1a6827ac8481b,
> > a3eb97bd80134ba07864ca00747466c02118aca1) ended up removing too much:
> > although rdtsc is now orderd WRT other loads, there's no protection
> > against the compiler reordering the loads of ->version with the loads of
> > other fields.
> > 
> > E.g. on my system gcc-5.3.1 generates code which loads ->system_time and
> > ->flags outside of the ->version test loop.
> > 
> > (Re)introduce the compiler barriers around accesses to the contents of
> > pvclock.  While at this, make the function a bit more compact by
> > removing unnecessary local variables.
> > 
> > Signed-off-by: Roman Kagan 
> > Cc: Thomas Gleixner 
> > Cc: Ingo Molnar 
> > Cc: "H. Peter Anvin" 
> > Cc: x...@kernel.org
> > Cc: Andy Lutomirski 
> > Cc: Borislav Petkov 
> > Cc: Paolo Bonzini 
> > Cc: sta...@vger.kernel.org
> > ---
> >  arch/x86/include/asm/pvclock.h | 17 +
> >  1 file changed, 5 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> > index fdcc040..65c4de2 100644
> > --- a/arch/x86/include/asm/pvclock.h
> > +++ b/arch/x86/include/asm/pvclock.h
> > @@ -80,18 +80,11 @@ static __always_inline
> >  unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
> >cycle_t *cycles, u8 *flags)
> >  {
> > -   unsigned version;
> > -   cycle_t ret, offset;
> > -   u8 ret_flags;
> > -
> > -   version = src->version;
> > -
> > -   offset = pvclock_get_nsec_offset(src);
> > -   ret = src->system_time + offset;
> > -   ret_flags = src->flags;
> > -
> > -   *cycles = ret;
> > -   *flags = ret_flags;
> > +   unsigned version = src->version;
> > +   barrier();
> > +   *cycles = src->system_time + pvclock_get_nsec_offset(src);
> > +   *flags = src->flags;
> > +   barrier();
> > return version;
> 
> I have a similar patchset in my mbox starting here:
> 
> https://lkml.kernel.org/r/1464329832-4638-1-git-send-email-mngh...@gmail.com
> 
> Care to take a look?

Hi, Boris.

I had a vocation last several days, and sorry for replying it
later.

As Roman said in this thread, both his and mine are similar way
to fix this inconsistent issue, and I have a wrapper function
smp_rmb in my patch which the root function is barriers in x86
as well.


Thanks
Minfei

Re: [PATCH V2] Use MACRO UINT_MAX instead of actual value

[Minfei Huang]

Ping. Any comment is appreciate.

Thanks
Minfei

On 05/17/16 at 03:58P, Minfei Huang wrote:
> It's more elegant to use MACRO UINT_MAX to represent the max value of
> type unsigned int. So replace the actual value by using this MACRO.
> 
> Signed-off-by: Minfei Huang <mngh...@gmail.com>
> ---
> v1:
> - fix typo
> ---
>  drivers/nvme/host/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 643f457..2c0bb13 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -597,7 +597,7 @@ static void nvme_config_discard(struct nvme_ns *ns)
>  
>   ns->queue->limits.discard_alignment = logical_block_size;
>   ns->queue->limits.discard_granularity = logical_block_size;
> - blk_queue_max_discard_sectors(ns->queue, 0x);
> + blk_queue_max_discard_sectors(ns->queue, UINT_MAX);
>   queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
>  }
>  
> -- 
> 2.6.3
> 

Re: [PATCH V2] Use MACRO UINT_MAX instead of actual value

[Minfei Huang]

Ping. Any comment is appreciate.

Thanks
Minfei

On 05/17/16 at 03:58P, Minfei Huang wrote:
> It's more elegant to use MACRO UINT_MAX to represent the max value of
> type unsigned int. So replace the actual value by using this MACRO.
> 
> Signed-off-by: Minfei Huang 
> ---
> v1:
> - fix typo
> ---
>  drivers/nvme/host/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 643f457..2c0bb13 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -597,7 +597,7 @@ static void nvme_config_discard(struct nvme_ns *ns)
>  
>   ns->queue->limits.discard_alignment = logical_block_size;
>   ns->queue->limits.discard_granularity = logical_block_size;
> - blk_queue_max_discard_sectors(ns->queue, 0x);
> + blk_queue_max_discard_sectors(ns->queue, UINT_MAX);
>   queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
>  }
>  
> -- 
> 2.6.3
> 

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Minfei Huang]

ping. Any comment is appreciate.

Thanks
Minfei

On 05/28/16 at 08:27P, Minfei Huang wrote:
> There is a generic function __pvclock_read_cycles to be used to get both
> flags and cycles. For function pvclock_read_flags, it's useless to get
> cycles value. To make this function be more effective, get this variable
> flags directly in function.
> 
> Signed-off-by: Minfei Huang <mngh...@gmail.com>
> ---
> v1:
> - Get rid of __pvclock_read_cycles according to Andy's suggestion
> ---
>  arch/x86/kernel/pvclock.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
> index 7f82fe0..06c58ce 100644
> --- a/arch/x86/kernel/pvclock.c
> +++ b/arch/x86/kernel/pvclock.c
> @@ -61,11 +61,14 @@ void pvclock_resume(void)
>  u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
>  {
>   unsigned version;
> - cycle_t ret;
>   u8 flags;
>  
>   do {
> - version = __pvclock_read_cycles(src, , );
> + version = src->version;
> + /* Make the latest version visible */
> + smp_rmb();
> +
> + flags = src->flags;
>   /* Make sure that the version double-check is last. */
>   smp_rmb();
>   } while ((src->version & 1) || version != src->version);
> -- 
> 2.6.3
> 

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Minfei Huang]

ping. Any comment is appreciate.

Thanks
Minfei

On 05/28/16 at 08:27P, Minfei Huang wrote:
> There is a generic function __pvclock_read_cycles to be used to get both
> flags and cycles. For function pvclock_read_flags, it's useless to get
> cycles value. To make this function be more effective, get this variable
> flags directly in function.
> 
> Signed-off-by: Minfei Huang 
> ---
> v1:
> - Get rid of __pvclock_read_cycles according to Andy's suggestion
> ---
>  arch/x86/kernel/pvclock.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
> index 7f82fe0..06c58ce 100644
> --- a/arch/x86/kernel/pvclock.c
> +++ b/arch/x86/kernel/pvclock.c
> @@ -61,11 +61,14 @@ void pvclock_resume(void)
>  u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
>  {
>   unsigned version;
> - cycle_t ret;
>   u8 flags;
>  
>   do {
> - version = __pvclock_read_cycles(src, , );
> + version = src->version;
> + /* Make the latest version visible */
> + smp_rmb();
> +
> + flags = src->flags;
>   /* Make sure that the version double-check is last. */
>   smp_rmb();
>   } while ((src->version & 1) || version != src->version);
> -- 
> 2.6.3
> 

[PATCH] loop: Make user notify for adding loop device failed

[Minfei Huang]

There is no error number returned if loop driver fails in function
alloc_disk to add new loop device. Add a correct error number to make
user notify in this case.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 drivers/block/loop.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 1fa8cc2..2caaf6f 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1765,6 +1765,7 @@ static int loop_add(struct loop_device **l, int i)
 */
queue_flag_set_unlocked(QUEUE_FLAG_NOMERGES, lo->lo_queue);
 
+   err = -ENOMEM;
disk = lo->lo_disk = alloc_disk(1 << part_shift);
if (!disk)
goto out_free_queue;
-- 
2.7.4 (Apple Git-66)

[PATCH] loop: Make user notify for adding loop device failed

[Minfei Huang]

There is no error number returned if loop driver fails in function
alloc_disk to add new loop device. Add a correct error number to make
user notify in this case.

Signed-off-by: Minfei Huang 
---
 drivers/block/loop.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 1fa8cc2..2caaf6f 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1765,6 +1765,7 @@ static int loop_add(struct loop_device **l, int i)
 */
queue_flag_set_unlocked(QUEUE_FLAG_NOMERGES, lo->lo_queue);
 
+   err = -ENOMEM;
disk = lo->lo_disk = alloc_disk(1 << part_shift);
if (!disk)
goto out_free_queue;
-- 
2.7.4 (Apple Git-66)

[PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Minfei Huang]

There is a generic function __pvclock_read_cycles to be used to get both
flags and cycles. For function pvclock_read_flags, it's useless to get
cycles value. To make this function be more effective, get this variable
flags directly in function.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
v1:
- Get rid of __pvclock_read_cycles according to Andy's suggestion
---
 arch/x86/kernel/pvclock.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 7f82fe0..06c58ce 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -61,11 +61,14 @@ void pvclock_resume(void)
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
unsigned version;
-   cycle_t ret;
u8 flags;
 
do {
-   version = __pvclock_read_cycles(src, , );
+   version = src->version;
+   /* Make the latest version visible */
+   smp_rmb();
+
+   flags = src->flags;
/* Make sure that the version double-check is last. */
smp_rmb();
} while ((src->version & 1) || version != src->version);
-- 
2.6.3

[PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

[Minfei Huang]

There is a generic function __pvclock_read_cycles to be used to get both
flags and cycles. For function pvclock_read_flags, it's useless to get
cycles value. To make this function be more effective, get this variable
flags directly in function.

Signed-off-by: Minfei Huang 
---
v1:
- Get rid of __pvclock_read_cycles according to Andy's suggestion
---
 arch/x86/kernel/pvclock.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 7f82fe0..06c58ce 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -61,11 +61,14 @@ void pvclock_resume(void)
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
unsigned version;
-   cycle_t ret;
u8 flags;
 
do {
-   version = __pvclock_read_cycles(src, , );
+   version = src->version;
+   /* Make the latest version visible */
+   smp_rmb();
+
+   flags = src->flags;
/* Make sure that the version double-check is last. */
smp_rmb();
} while ((src->version & 1) || version != src->version);
-- 
2.6.3

Re: [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags

[Minfei Huang]

On 05/27/16 at 06:06P, Paolo Bonzini wrote:
> 
> 
> On 27/05/2016 17:40, Andy Lutomirski wrote:
> > On Thu, May 26, 2016 at 11:17 PM, Minfei Huang <mngh...@gmail.com> wrote:
> >> There is a generic function __pvclock_read_cycles to be used to get both
> >> flags and cycles. For function pvclock_read_flags, it's useless to get
> >> cycles value. To make this function be more effective, add a new wrapper
> >> function to only get variable flags.
> > 
> > Why not just get rid of this function entirely?
> 
> I agree.  If you want to abstract the retry logic, perhaps you can add

Hmm, I will get rid of pvclock_read_flags, and repost it.

Thanks
Minfei

> pvclock_read_{begin,retry} functions like seqlock_read_{begin,retry}?
> 
> Thanks,
> 
> Paolo

Re: [PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags

[Minfei Huang]

On 05/27/16 at 06:06P, Paolo Bonzini wrote:
> 
> 
> On 27/05/2016 17:40, Andy Lutomirski wrote:
> > On Thu, May 26, 2016 at 11:17 PM, Minfei Huang  wrote:
> >> There is a generic function __pvclock_read_cycles to be used to get both
> >> flags and cycles. For function pvclock_read_flags, it's useless to get
> >> cycles value. To make this function be more effective, add a new wrapper
> >> function to only get variable flags.
> > 
> > Why not just get rid of this function entirely?
> 
> I agree.  If you want to abstract the retry logic, perhaps you can add

Hmm, I will get rid of pvclock_read_flags, and repost it.

Thanks
Minfei

> pvclock_read_{begin,retry} functions like seqlock_read_{begin,retry}?
> 
> Thanks,
> 
> Paolo

[PATCH 1/3] pvclock: Add CPU barries to get correct version value

[Minfei Huang]

Protocol for the "version" fields is: hypervisor raises it (making it
uneven) before it starts updating the fields and raises it again (making
it even) when it is done.  Thus the guest can make sure the time values
it got are consistent by checking the version before and after reading
them.

Add CPU barries after getting version value just like what function
vread_pvclock does, because all of callees in this function is inline.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 arch/x86/include/asm/pvclock.h | 2 ++
 arch/x86/kernel/pvclock.c  | 4 
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index fdcc040..538ae94 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -85,6 +85,8 @@ unsigned __pvclock_read_cycles(const struct 
pvclock_vcpu_time_info *src,
u8 ret_flags;
 
version = src->version;
+   /* Make the latest version visible */
+   smp_rmb();
 
offset = pvclock_get_nsec_offset(src);
ret = src->system_time + offset;
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 99bfc02..7f82fe0 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -66,6 +66,8 @@ u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 
do {
version = __pvclock_read_cycles(src, , );
+   /* Make sure that the version double-check is last. */
+   smp_rmb();
} while ((src->version & 1) || version != src->version);
 
return flags & valid_flags;
@@ -80,6 +82,8 @@ cycle_t pvclock_clocksource_read(struct 
pvclock_vcpu_time_info *src)
 
do {
version = __pvclock_read_cycles(src, , );
+   /* Make sure that the version double-check is last. */
+   smp_rmb();
} while ((src->version & 1) || version != src->version);
 
if (unlikely((flags & PVCLOCK_GUEST_STOPPED) != 0)) {
-- 
2.6.3

[PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags

[Minfei Huang]

There is a generic function __pvclock_read_cycles to be used to get both
flags and cycles. For function pvclock_read_flags, it's useless to get
cycles value. To make this function be more effective, add a new wrapper
function to only get variable flags.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 arch/x86/include/asm/pvclock.h | 12 
 arch/x86/kernel/pvclock.c  |  3 +--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 7c1c895..3c4e5b7 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -88,6 +88,18 @@ unsigned __pvclock_read_cycles(const struct 
pvclock_vcpu_time_info *src,
return version;
 }
 
+static __always_inline unsigned int __pvclock_read_flags(
+   const struct pvclock_vcpu_time_info *src, u8 *flags)
+{
+   unsigned int version;
+
+   version = src->version;
+   /* Make the latest version visible */
+   smp_rmb();
+   *flags = src->flags;
+   return version;
+}
+
 struct pvclock_vsyscall_time_info {
struct pvclock_vcpu_time_info pvti;
 } __attribute__((__aligned__(SMP_CACHE_BYTES)));
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 7f82fe0..138772c 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -61,11 +61,10 @@ void pvclock_resume(void)
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
unsigned version;
-   cycle_t ret;
u8 flags;
 
do {
-   version = __pvclock_read_cycles(src, , );
+   version = __pvclock_read_flags(src, );
/* Make sure that the version double-check is last. */
smp_rmb();
} while ((src->version & 1) || version != src->version);
-- 
2.6.3

[PATCH 2/3] pvclock: Cleanup to remove function pvclock_get_nsec_offset

[Minfei Huang]

Function __pvclock_read_cycles is short enough, so there is no need to
have another function pvclock_get_nsec_offset to calculate tsc delta.
It's better to combine it into function __pvclock_read_cycles.

Remove useless variables in function __pvclock_read_cycles.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 arch/x86/include/asm/pvclock.h | 23 +++
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 538ae94..7c1c895 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -69,31 +69,22 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 
mul_frac, int shift)
 }
 
 static __always_inline
-u64 pvclock_get_nsec_offset(const struct pvclock_vcpu_time_info *src)
-{
-   u64 delta = rdtsc_ordered() - src->tsc_timestamp;
-   return pvclock_scale_delta(delta, src->tsc_to_system_mul,
-  src->tsc_shift);
-}
-
-static __always_inline
 unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
   cycle_t *cycles, u8 *flags)
 {
unsigned version;
-   cycle_t ret, offset;
-   u8 ret_flags;
+   cycle_t offset;
+   u64 delta;
 
version = src->version;
/* Make the latest version visible */
smp_rmb();
 
-   offset = pvclock_get_nsec_offset(src);
-   ret = src->system_time + offset;
-   ret_flags = src->flags;
-
-   *cycles = ret;
-   *flags = ret_flags;
+   delta = rdtsc_ordered() - src->tsc_timestamp;
+   offset = pvclock_scale_delta(delta, src->tsc_to_system_mul,
+  src->tsc_shift);
+   *cycles = src->system_time + offset;
+   *flags = src->flags;
return version;
 }
 
-- 
2.6.3

[PATCH 1/3] pvclock: Add CPU barries to get correct version value

[Minfei Huang]

Protocol for the "version" fields is: hypervisor raises it (making it
uneven) before it starts updating the fields and raises it again (making
it even) when it is done.  Thus the guest can make sure the time values
it got are consistent by checking the version before and after reading
them.

Add CPU barries after getting version value just like what function
vread_pvclock does, because all of callees in this function is inline.

Signed-off-by: Minfei Huang 
---
 arch/x86/include/asm/pvclock.h | 2 ++
 arch/x86/kernel/pvclock.c  | 4 
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index fdcc040..538ae94 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -85,6 +85,8 @@ unsigned __pvclock_read_cycles(const struct 
pvclock_vcpu_time_info *src,
u8 ret_flags;
 
version = src->version;
+   /* Make the latest version visible */
+   smp_rmb();
 
offset = pvclock_get_nsec_offset(src);
ret = src->system_time + offset;
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 99bfc02..7f82fe0 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -66,6 +66,8 @@ u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 
do {
version = __pvclock_read_cycles(src, , );
+   /* Make sure that the version double-check is last. */
+   smp_rmb();
} while ((src->version & 1) || version != src->version);
 
return flags & valid_flags;
@@ -80,6 +82,8 @@ cycle_t pvclock_clocksource_read(struct 
pvclock_vcpu_time_info *src)
 
do {
version = __pvclock_read_cycles(src, , );
+   /* Make sure that the version double-check is last. */
+   smp_rmb();
} while ((src->version & 1) || version != src->version);
 
if (unlikely((flags & PVCLOCK_GUEST_STOPPED) != 0)) {
-- 
2.6.3

[PATCH 3/3] pvclock: Add a new wrapper function to only get variable flags

[Minfei Huang]

There is a generic function __pvclock_read_cycles to be used to get both
flags and cycles. For function pvclock_read_flags, it's useless to get
cycles value. To make this function be more effective, add a new wrapper
function to only get variable flags.

Signed-off-by: Minfei Huang 
---
 arch/x86/include/asm/pvclock.h | 12 
 arch/x86/kernel/pvclock.c  |  3 +--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 7c1c895..3c4e5b7 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -88,6 +88,18 @@ unsigned __pvclock_read_cycles(const struct 
pvclock_vcpu_time_info *src,
return version;
 }
 
+static __always_inline unsigned int __pvclock_read_flags(
+   const struct pvclock_vcpu_time_info *src, u8 *flags)
+{
+   unsigned int version;
+
+   version = src->version;
+   /* Make the latest version visible */
+   smp_rmb();
+   *flags = src->flags;
+   return version;
+}
+
 struct pvclock_vsyscall_time_info {
struct pvclock_vcpu_time_info pvti;
 } __attribute__((__aligned__(SMP_CACHE_BYTES)));
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 7f82fe0..138772c 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -61,11 +61,10 @@ void pvclock_resume(void)
 u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
 {
unsigned version;
-   cycle_t ret;
u8 flags;
 
do {
-   version = __pvclock_read_cycles(src, , );
+   version = __pvclock_read_flags(src, );
/* Make sure that the version double-check is last. */
smp_rmb();
} while ((src->version & 1) || version != src->version);
-- 
2.6.3

[PATCH 2/3] pvclock: Cleanup to remove function pvclock_get_nsec_offset

[Minfei Huang]

Function __pvclock_read_cycles is short enough, so there is no need to
have another function pvclock_get_nsec_offset to calculate tsc delta.
It's better to combine it into function __pvclock_read_cycles.

Remove useless variables in function __pvclock_read_cycles.

Signed-off-by: Minfei Huang 
---
 arch/x86/include/asm/pvclock.h | 23 +++
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 538ae94..7c1c895 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -69,31 +69,22 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 
mul_frac, int shift)
 }
 
 static __always_inline
-u64 pvclock_get_nsec_offset(const struct pvclock_vcpu_time_info *src)
-{
-   u64 delta = rdtsc_ordered() - src->tsc_timestamp;
-   return pvclock_scale_delta(delta, src->tsc_to_system_mul,
-  src->tsc_shift);
-}
-
-static __always_inline
 unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
   cycle_t *cycles, u8 *flags)
 {
unsigned version;
-   cycle_t ret, offset;
-   u8 ret_flags;
+   cycle_t offset;
+   u64 delta;
 
version = src->version;
/* Make the latest version visible */
smp_rmb();
 
-   offset = pvclock_get_nsec_offset(src);
-   ret = src->system_time + offset;
-   ret_flags = src->flags;
-
-   *cycles = ret;
-   *flags = ret_flags;
+   delta = rdtsc_ordered() - src->tsc_timestamp;
+   offset = pvclock_scale_delta(delta, src->tsc_to_system_mul,
+  src->tsc_shift);
+   *cycles = src->system_time + offset;
+   *flags = src->flags;
return version;
 }
 
-- 
2.6.3

[PATCH V2] MAINTAINERS: add kexec_core.c and kexec_file.c

[Minfei Huang]

>From below commits, kexec.c is split to kexec.c, kexec_file.c and
kexec_core.c.

commit a43cac0d9dc2 ("kexec: split kexec_file syscall code to
kexec_file.c")
commit 2965faa5e03d ("kexec: split kexec_load syscall from kexec core
code")

Both kexec_file.c and kexec_core.c are still belong to kexec component.
In order to get correct mail lists by using script get_maintainer.pl,
add these files to MAINTAINERS.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
v1:
- use kexec* to match all of kexec related file
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3302006..9c18fa8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6559,7 +6559,7 @@ L:ke...@lists.infradead.org
 S: Maintained
 F: include/linux/kexec.h
 F: include/uapi/linux/kexec.h
-F: kernel/kexec.c
+F: kernel/kexec*
 
 KEYS/KEYRINGS:
 M: David Howells <dhowe...@redhat.com>
-- 
2.6.3

[PATCH V2] MAINTAINERS: add kexec_core.c and kexec_file.c

[Minfei Huang]

>From below commits, kexec.c is split to kexec.c, kexec_file.c and
kexec_core.c.

commit a43cac0d9dc2 ("kexec: split kexec_file syscall code to
kexec_file.c")
commit 2965faa5e03d ("kexec: split kexec_load syscall from kexec core
code")

Both kexec_file.c and kexec_core.c are still belong to kexec component.
In order to get correct mail lists by using script get_maintainer.pl,
add these files to MAINTAINERS.

Signed-off-by: Minfei Huang 
---
v1:
- use kexec* to match all of kexec related file
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3302006..9c18fa8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6559,7 +6559,7 @@ L:ke...@lists.infradead.org
 S: Maintained
 F: include/linux/kexec.h
 F: include/uapi/linux/kexec.h
-F: kernel/kexec.c
+F: kernel/kexec*
 
 KEYS/KEYRINGS:
 M: David Howells 
-- 
2.6.3

Re: [PATCH] MAINTAINERS: add kexec_core.c and kexec_file.c

[Minfei Huang]

On 05/25/16 at 05:54P, Joe Perches wrote:
> On Wed, 2016-05-25 at 20:49 +0800, Minfei Huang wrote:
> > 
> > From below commits, kexec.c is split to kexec.c, kexec_file.c and
> > kexec_core.c.
> > 
> > commit a43cac0d9dc2 ("kexec: split kexec_file syscall code to
> > kexec_file.c")
> > commit 2965faa5e03d ("kexec: split kexec_load syscall from kexec core
> > code")
> > 
> > Both kexec_file.c and kexec_core.c are still belong to kexec component.
> > In order to get correct mail lists by using script get_maintainer.pl,
> > add these files to MAINTAINERS.
> > 
> > Signed-off-by: Minfei Huang <mngh...@gmail.com>
> > ---
> > ?MAINTAINERS | 2 ++
> > ?1 file changed, 2 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> []
> > @@ -6560,6 +6560,8 @@ S:Maintained
> > ?F: include/linux/kexec.h
> > ?F: include/uapi/linux/kexec.h
> > ?F: kernel/kexec.c
> > +F: kernel/kexec_core.c
> > +F: kernel/kexec_file.c
> 
> F:kernel/kexec*
> 
> would be simpler and it also matches kernel/kexec_internal.h

Thanks. Will update it.

Thanks
Minfei

Re: [PATCH] MAINTAINERS: add kexec_core.c and kexec_file.c

[Minfei Huang]

On 05/25/16 at 05:54P, Joe Perches wrote:
> On Wed, 2016-05-25 at 20:49 +0800, Minfei Huang wrote:
> > 
> > From below commits, kexec.c is split to kexec.c, kexec_file.c and
> > kexec_core.c.
> > 
> > commit a43cac0d9dc2 ("kexec: split kexec_file syscall code to
> > kexec_file.c")
> > commit 2965faa5e03d ("kexec: split kexec_load syscall from kexec core
> > code")
> > 
> > Both kexec_file.c and kexec_core.c are still belong to kexec component.
> > In order to get correct mail lists by using script get_maintainer.pl,
> > add these files to MAINTAINERS.
> > 
> > Signed-off-by: Minfei Huang 
> > ---
> > ?MAINTAINERS | 2 ++
> > ?1 file changed, 2 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> []
> > @@ -6560,6 +6560,8 @@ S:Maintained
> > ?F: include/linux/kexec.h
> > ?F: include/uapi/linux/kexec.h
> > ?F: kernel/kexec.c
> > +F: kernel/kexec_core.c
> > +F: kernel/kexec_file.c
> 
> F:kernel/kexec*
> 
> would be simpler and it also matches kernel/kexec_internal.h

Thanks. Will update it.

Thanks
Minfei

[PATCH] MAINTAINERS: add kexec_core.c and kexec_file.c

[Minfei Huang]

>From below commits, kexec.c is split to kexec.c, kexec_file.c and
kexec_core.c.

commit a43cac0d9dc2 ("kexec: split kexec_file syscall code to
kexec_file.c")
commit 2965faa5e03d ("kexec: split kexec_load syscall from kexec core
code")

Both kexec_file.c and kexec_core.c are still belong to kexec component.
In order to get correct mail lists by using script get_maintainer.pl,
add these files to MAINTAINERS.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3302006..5f33f71 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6560,6 +6560,8 @@ S:Maintained
 F: include/linux/kexec.h
 F: include/uapi/linux/kexec.h
 F: kernel/kexec.c
+F: kernel/kexec_core.c
+F: kernel/kexec_file.c
 
 KEYS/KEYRINGS:
 M: David Howells <dhowe...@redhat.com>
-- 
2.6.3

[PATCH] MAINTAINERS: add kexec_core.c and kexec_file.c

[Minfei Huang]

>From below commits, kexec.c is split to kexec.c, kexec_file.c and
kexec_core.c.

commit a43cac0d9dc2 ("kexec: split kexec_file syscall code to
kexec_file.c")
commit 2965faa5e03d ("kexec: split kexec_load syscall from kexec core
code")

Both kexec_file.c and kexec_core.c are still belong to kexec component.
In order to get correct mail lists by using script get_maintainer.pl,
add these files to MAINTAINERS.

Signed-off-by: Minfei Huang 
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3302006..5f33f71 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6560,6 +6560,8 @@ S:Maintained
 F: include/linux/kexec.h
 F: include/uapi/linux/kexec.h
 F: kernel/kexec.c
+F: kernel/kexec_core.c
+F: kernel/kexec_file.c
 
 KEYS/KEYRINGS:
 M: David Howells 
-- 
2.6.3

[PATCH] kexec: Return error number directly

[Minfei Huang]

This is a cleanup patch to make kexec more clear to return error number
directly. The variable result is useless, because there is no other
function's return value assignes to it. So remove it.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 kernel/kexec_core.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 56b3ed0..23311c8 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -147,7 +147,7 @@ static struct page *kimage_alloc_page(struct kimage *image,
 
 int sanity_check_segment_list(struct kimage *image)
 {
-   int result, i;
+   int i;
unsigned long nr_segments = image->nr_segments;
 
/*
@@ -163,16 +163,15 @@ int sanity_check_segment_list(struct kimage *image)
 * simply because addresses are changed to page size
 * granularity.
 */
-   result = -EADDRNOTAVAIL;
for (i = 0; i < nr_segments; i++) {
unsigned long mstart, mend;
 
mstart = image->segment[i].mem;
mend   = mstart + image->segment[i].memsz;
if ((mstart & ~PAGE_MASK) || (mend & ~PAGE_MASK))
-   return result;
+   return -EADDRNOTAVAIL;
if (mend >= KEXEC_DESTINATION_MEMORY_LIMIT)
-   return result;
+   return -EADDRNOTAVAIL;
}
 
/* Verify our destination addresses do not overlap.
@@ -180,7 +179,6 @@ int sanity_check_segment_list(struct kimage *image)
 * through very weird things can happen with no
 * easy explanation as one segment stops on another.
 */
-   result = -EINVAL;
for (i = 0; i < nr_segments; i++) {
unsigned long mstart, mend;
unsigned long j;
@@ -194,7 +192,7 @@ int sanity_check_segment_list(struct kimage *image)
pend   = pstart + image->segment[j].memsz;
/* Do the segments overlap ? */
if ((mend > pstart) && (mstart < pend))
-   return result;
+   return -EINVAL;
}
}
 
@@ -203,10 +201,9 @@ int sanity_check_segment_list(struct kimage *image)
 * and it is easier to check up front than to be surprised
 * later on.
 */
-   result = -EINVAL;
for (i = 0; i < nr_segments; i++) {
if (image->segment[i].bufsz > image->segment[i].memsz)
-   return result;
+   return -EINVAL;
}
 
/*
@@ -220,7 +217,6 @@ int sanity_check_segment_list(struct kimage *image)
 */
 
if (image->type == KEXEC_TYPE_CRASH) {
-   result = -EADDRNOTAVAIL;
for (i = 0; i < nr_segments; i++) {
unsigned long mstart, mend;
 
@@ -229,7 +225,7 @@ int sanity_check_segment_list(struct kimage *image)
/* Ensure we are within the crash kernel limits */
if ((mstart < crashk_res.start) ||
(mend > crashk_res.end))
-   return result;
+   return -EADDRNOTAVAIL;
}
}
 
-- 
2.6.3

[PATCH] kexec: Return error number directly

[Minfei Huang]

This is a cleanup patch to make kexec more clear to return error number
directly. The variable result is useless, because there is no other
function's return value assignes to it. So remove it.

Signed-off-by: Minfei Huang 
---
 kernel/kexec_core.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
index 56b3ed0..23311c8 100644
--- a/kernel/kexec_core.c
+++ b/kernel/kexec_core.c
@@ -147,7 +147,7 @@ static struct page *kimage_alloc_page(struct kimage *image,
 
 int sanity_check_segment_list(struct kimage *image)
 {
-   int result, i;
+   int i;
unsigned long nr_segments = image->nr_segments;
 
/*
@@ -163,16 +163,15 @@ int sanity_check_segment_list(struct kimage *image)
 * simply because addresses are changed to page size
 * granularity.
 */
-   result = -EADDRNOTAVAIL;
for (i = 0; i < nr_segments; i++) {
unsigned long mstart, mend;
 
mstart = image->segment[i].mem;
mend   = mstart + image->segment[i].memsz;
if ((mstart & ~PAGE_MASK) || (mend & ~PAGE_MASK))
-   return result;
+   return -EADDRNOTAVAIL;
if (mend >= KEXEC_DESTINATION_MEMORY_LIMIT)
-   return result;
+   return -EADDRNOTAVAIL;
}
 
/* Verify our destination addresses do not overlap.
@@ -180,7 +179,6 @@ int sanity_check_segment_list(struct kimage *image)
 * through very weird things can happen with no
 * easy explanation as one segment stops on another.
 */
-   result = -EINVAL;
for (i = 0; i < nr_segments; i++) {
unsigned long mstart, mend;
unsigned long j;
@@ -194,7 +192,7 @@ int sanity_check_segment_list(struct kimage *image)
pend   = pstart + image->segment[j].memsz;
/* Do the segments overlap ? */
if ((mend > pstart) && (mstart < pend))
-   return result;
+   return -EINVAL;
}
}
 
@@ -203,10 +201,9 @@ int sanity_check_segment_list(struct kimage *image)
 * and it is easier to check up front than to be surprised
 * later on.
 */
-   result = -EINVAL;
for (i = 0; i < nr_segments; i++) {
if (image->segment[i].bufsz > image->segment[i].memsz)
-   return result;
+   return -EINVAL;
}
 
/*
@@ -220,7 +217,6 @@ int sanity_check_segment_list(struct kimage *image)
 */
 
if (image->type == KEXEC_TYPE_CRASH) {
-   result = -EADDRNOTAVAIL;
for (i = 0; i < nr_segments; i++) {
unsigned long mstart, mend;
 
@@ -229,7 +225,7 @@ int sanity_check_segment_list(struct kimage *image)
/* Ensure we are within the crash kernel limits */
if ((mstart < crashk_res.start) ||
(mend > crashk_res.end))
-   return result;
+   return -EADDRNOTAVAIL;
}
}
 
-- 
2.6.3

[PATCH V2] Use MACRO UINT_MAX instead of actual value

[Minfei Huang]

It's more elegant to use MACRO UINT_MAX to represent the max value of
type unsigned int. So replace the actual value by using this MACRO.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
v1:
- fix typo
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 643f457..2c0bb13 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -597,7 +597,7 @@ static void nvme_config_discard(struct nvme_ns *ns)
 
ns->queue->limits.discard_alignment = logical_block_size;
ns->queue->limits.discard_granularity = logical_block_size;
-   blk_queue_max_discard_sectors(ns->queue, 0x);
+   blk_queue_max_discard_sectors(ns->queue, UINT_MAX);
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
 }
 
-- 
2.6.3

[PATCH V2] Use MACRO UINT_MAX instead of actual value

[Minfei Huang]

It's more elegant to use MACRO UINT_MAX to represent the max value of
type unsigned int. So replace the actual value by using this MACRO.

Signed-off-by: Minfei Huang 
---
v1:
- fix typo
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 643f457..2c0bb13 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -597,7 +597,7 @@ static void nvme_config_discard(struct nvme_ns *ns)
 
ns->queue->limits.discard_alignment = logical_block_size;
ns->queue->limits.discard_granularity = logical_block_size;
-   blk_queue_max_discard_sectors(ns->queue, 0x);
+   blk_queue_max_discard_sectors(ns->queue, UINT_MAX);
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
 }
 
-- 
2.6.3

Re: [PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Minfei Huang]

On 05/11/16 at 04:24P, Paolo Bonzini wrote:
> 
> 
> On 30/04/2016 23:57, Andy Lutomirski wrote:
> >> > Should we kill __pvclock_read_cycles in favor of vread_pvclock? It looks
> >> > doable at a quick scan...
> >> >
> > The in-kernel version might have to be a bit different because it
> > needs to handle the !stable case.  If !stable, it should just use the
> > current CPU's copy which means that, realistically, it should just
> > get_cpu and use the local copy unconditionally.  Other than that, it
> > could look a lot like the vread_pvclock variant.
> > 
> > But I agree, the current thing is incomprehensible.
> 
> It also lacks smp_rmb()s.  One is more or less implicit in rdtsc, but
> you need one to separate __pvclock_read_cycles's reads of src->foo from
> pvclock_read_flags's read of src->version.
> 
> Minfei, would you like to take a look?

Sure. I will take a look about this issue.

Thanks
Minfei

> 
> Paolo

Re: [PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Minfei Huang]

On 05/11/16 at 04:24P, Paolo Bonzini wrote:
> 
> 
> On 30/04/2016 23:57, Andy Lutomirski wrote:
> >> > Should we kill __pvclock_read_cycles in favor of vread_pvclock? It looks
> >> > doable at a quick scan...
> >> >
> > The in-kernel version might have to be a bit different because it
> > needs to handle the !stable case.  If !stable, it should just use the
> > current CPU's copy which means that, realistically, it should just
> > get_cpu and use the local copy unconditionally.  Other than that, it
> > could look a lot like the vread_pvclock variant.
> > 
> > But I agree, the current thing is incomprehensible.
> 
> It also lacks smp_rmb()s.  One is more or less implicit in rdtsc, but
> you need one to separate __pvclock_read_cycles's reads of src->foo from
> pvclock_read_flags's read of src->version.
> 
> Minfei, would you like to take a look?

Sure. I will take a look about this issue.

Thanks
Minfei

> 
> Paolo

Re: [PATCH] Make clocksource insert entry more efficiently

[Minfei Huang]

On 05/02/16 at 10:39P, John Stultz wrote:
> On Mon, Apr 25, 2016 at 2:20 AM, Minfei Huang <mngh...@gmail.com> wrote:
> > It is unnecessary to continue looping the list, if we find there is an
> > entry that the value of rating is smaller than the new one. It is safe
> > to be out the loop, because all of entry are inserted in descending
> > order.
> >
> > Signed-off-by: Minfei Huang <mngh...@gmail.com>
> > ---
> 
> Thanks for sending this in. It looks reasonable to me, though not
> likely to gain a ton, as these are usually short lists, and we only
> really tinker with them at bootup. But yea. I'll put it on my toqueue
> list for testing.
> 
> Its maybe a little close to make it for 4.7, but we'll see.
> 
> Thanks again!
> -john

Thanks

Minfei

Re: [PATCH] Make clocksource insert entry more efficiently

[Minfei Huang]

On 05/02/16 at 10:39P, John Stultz wrote:
> On Mon, Apr 25, 2016 at 2:20 AM, Minfei Huang  wrote:
> > It is unnecessary to continue looping the list, if we find there is an
> > entry that the value of rating is smaller than the new one. It is safe
> > to be out the loop, because all of entry are inserted in descending
> > order.
> >
> > Signed-off-by: Minfei Huang 
> > ---
> 
> Thanks for sending this in. It looks reasonable to me, though not
> likely to gain a ton, as these are usually short lists, and we only
> really tinker with them at bootup. But yea. I'll put it on my toqueue
> list for testing.
> 
> Its maybe a little close to make it for 4.7, but we'll see.
> 
> Thanks again!
> -john

Thanks

Minfei

Re: [PATCH] Make clocksource insert entry more efficiently

[Minfei Huang]

Ping.

Any comment is appreciate.

Thanks
Minfei

On 04/25/16 at 05:20P, Minfei Huang wrote:
> It is unnecessary to continue looping the list, if we find there is an
> entry that the value of rating is smaller than the new one. It is safe
> to be out the loop, because all of entry are inserted in descending
> order.
> 
> Signed-off-by: Minfei Huang <mngh...@gmail.com>
> ---
>  kernel/time/clocksource.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index 56ece14..6a5a310 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -669,10 +669,12 @@ static void clocksource_enqueue(struct clocksource *cs)
>   struct list_head *entry = _list;
>   struct clocksource *tmp;
>  
> - list_for_each_entry(tmp, _list, list)
> + list_for_each_entry(tmp, _list, list) {
>   /* Keep track of the place, where to insert */
> - if (tmp->rating >= cs->rating)
> - entry = >list;
> + if (tmp->rating < cs->rating)
> + break;
> + entry = >list;
> + }
>   list_add(>list, entry);
>  }
>  
> -- 
> 2.6.3
> 

Re: [PATCH] Make clocksource insert entry more efficiently

[Minfei Huang]

Ping.

Any comment is appreciate.

Thanks
Minfei

On 04/25/16 at 05:20P, Minfei Huang wrote:
> It is unnecessary to continue looping the list, if we find there is an
> entry that the value of rating is smaller than the new one. It is safe
> to be out the loop, because all of entry are inserted in descending
> order.
> 
> Signed-off-by: Minfei Huang 
> ---
>  kernel/time/clocksource.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
> index 56ece14..6a5a310 100644
> --- a/kernel/time/clocksource.c
> +++ b/kernel/time/clocksource.c
> @@ -669,10 +669,12 @@ static void clocksource_enqueue(struct clocksource *cs)
>   struct list_head *entry = _list;
>   struct clocksource *tmp;
>  
> - list_for_each_entry(tmp, _list, list)
> + list_for_each_entry(tmp, _list, list) {
>   /* Keep track of the place, where to insert */
> - if (tmp->rating >= cs->rating)
> - entry = >list;
> + if (tmp->rating < cs->rating)
> + break;
> + entry = >list;
> + }
>   list_add(>list, entry);
>  }
>  
> -- 
> 2.6.3
> 

Re: [PATCH] Use MICRO UINT_MAX instead of actual value

[Minfei Huang]

Ping.

Any comment is appreciate.

Thanks
Minfei

On 04/25/16 at 11:13P, Minfei Huang wrote:
> It's more elegant to use MICRO UINT_MAX to represent the max value of
> type unsigned int. So replace the actual value by using this MICRO.
> 
> Signed-off-by: Minfei Huang <mngh...@gmail.com>
> ---
>  drivers/nvme/host/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 643f457..2c0bb13 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -597,7 +597,7 @@ static void nvme_config_discard(struct nvme_ns *ns)
>  
>   ns->queue->limits.discard_alignment = logical_block_size;
>   ns->queue->limits.discard_granularity = logical_block_size;
> - blk_queue_max_discard_sectors(ns->queue, 0x);
> + blk_queue_max_discard_sectors(ns->queue, UINT_MAX);
>   queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
>  }
>  
> -- 
> 2.6.3
> 

Re: [PATCH] Use MICRO UINT_MAX instead of actual value

[Minfei Huang]

Ping.

Any comment is appreciate.

Thanks
Minfei

On 04/25/16 at 11:13P, Minfei Huang wrote:
> It's more elegant to use MICRO UINT_MAX to represent the max value of
> type unsigned int. So replace the actual value by using this MICRO.
> 
> Signed-off-by: Minfei Huang 
> ---
>  drivers/nvme/host/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 643f457..2c0bb13 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -597,7 +597,7 @@ static void nvme_config_discard(struct nvme_ns *ns)
>  
>   ns->queue->limits.discard_alignment = logical_block_size;
>   ns->queue->limits.discard_granularity = logical_block_size;
> - blk_queue_max_discard_sectors(ns->queue, 0x);
> + blk_queue_max_discard_sectors(ns->queue, UINT_MAX);
>   queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
>  }
>  
> -- 
> 2.6.3
> 

Re: [PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Minfei Huang]

ping.

Any comment is appreciate.

Thanks
Minfei

On 04/25/16 at 02:53P, Minfei Huang wrote:
> The value of cycles and flags can be assigned directly without
> intermediate variables.
> 
> Remove the useless variables.
> 
> Signed-off-by: Minfei Huang <mngh...@gmail.com>
> ---
>  arch/x86/include/asm/pvclock.h | 15 ---
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> index fdcc040..fb95dac 100644
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
> @@ -80,19 +80,12 @@ static __always_inline
>  unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
>  cycle_t *cycles, u8 *flags)
>  {
> - unsigned version;
> - cycle_t ret, offset;
> - u8 ret_flags;
> -
> - version = src->version;
> + cycle_t offset;
>  
>   offset = pvclock_get_nsec_offset(src);
> - ret = src->system_time + offset;
> - ret_flags = src->flags;
> -
> - *cycles = ret;
> - *flags = ret_flags;
> - return version;
> + *cycles = src->system_time + offset;
> + *flags = src->flags;
> + return src->version;
>  }
>  
>  struct pvclock_vsyscall_time_info {
> -- 
> 2.6.3
> 

Re: [PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Minfei Huang]

ping.

Any comment is appreciate.

Thanks
Minfei

On 04/25/16 at 02:53P, Minfei Huang wrote:
> The value of cycles and flags can be assigned directly without
> intermediate variables.
> 
> Remove the useless variables.
> 
> Signed-off-by: Minfei Huang 
> ---
>  arch/x86/include/asm/pvclock.h | 15 ---
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> index fdcc040..fb95dac 100644
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
> @@ -80,19 +80,12 @@ static __always_inline
>  unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
>  cycle_t *cycles, u8 *flags)
>  {
> - unsigned version;
> - cycle_t ret, offset;
> - u8 ret_flags;
> -
> - version = src->version;
> + cycle_t offset;
>  
>   offset = pvclock_get_nsec_offset(src);
> - ret = src->system_time + offset;
> - ret_flags = src->flags;
> -
> - *cycles = ret;
> - *flags = ret_flags;
> - return version;
> + *cycles = src->system_time + offset;
> + *flags = src->flags;
> + return src->version;
>  }
>  
>  struct pvclock_vsyscall_time_info {
> -- 
> 2.6.3
> 

Re: [PATCH] Use existing helper to convert "on/off" to boolean

[Minfei Huang]

On 04/29/16 at 02:21P, Andrew Morton wrote:
> On Fri, 29 Apr 2016 13:47:04 +0800 Minfei Huang <mngh...@gmail.com> wrote:
> 
> > It's more convenient to use existing function helper to convert string
> > "on/off" to boolean.
> > 
> > ...
> >
> > --- a/lib/kstrtox.c
> > +++ b/lib/kstrtox.c
> > @@ -326,7 +326,7 @@ EXPORT_SYMBOL(kstrtos8);
> >   * @s: input string
> >   * @res: result
> >   *
> > - * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
> > + * This routine returns 0 if the first character is one of 'Yy1Nn0', or
> 
> That isn't actually a typo.  "iff" is shorthand for "if and only if". 
> ie: kstrtobool() will not return 0 in any other case.
> 
> Use of "iff" is a bit pretentious but I guess it does convey some
> conceivably useful info.
> 

Got it. Thanks for your explanation.

Thanks
Minfei

Re: [PATCH] Use existing helper to convert "on/off" to boolean

[Minfei Huang]

On 04/29/16 at 02:21P, Andrew Morton wrote:
> On Fri, 29 Apr 2016 13:47:04 +0800 Minfei Huang  wrote:
> 
> > It's more convenient to use existing function helper to convert string
> > "on/off" to boolean.
> > 
> > ...
> >
> > --- a/lib/kstrtox.c
> > +++ b/lib/kstrtox.c
> > @@ -326,7 +326,7 @@ EXPORT_SYMBOL(kstrtos8);
> >   * @s: input string
> >   * @res: result
> >   *
> > - * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
> > + * This routine returns 0 if the first character is one of 'Yy1Nn0', or
> 
> That isn't actually a typo.  "iff" is shorthand for "if and only if". 
> ie: kstrtobool() will not return 0 in any other case.
> 
> Use of "iff" is a bit pretentious but I guess it does convey some
> conceivably useful info.
> 

Got it. Thanks for your explanation.

Thanks
Minfei

Re: [RFC PATCH v2 06/18] x86: dump_trace() error handling

[Minfei Huang]

On 04/28/16 at 03:44P, Josh Poimboeuf wrote:
> In preparation for being able to determine whether a given stack trace
> is reliable, allow the stacktrace_ops functions to propagate errors to
> dump_trace().

Hi, Josh.

Have you considered to make walk_stack function as non-return function,
since there is no obvious error during detecting the frame points?

Thanks
Minfei

Re: [RFC PATCH v2 06/18] x86: dump_trace() error handling

[Minfei Huang]

On 04/28/16 at 03:44P, Josh Poimboeuf wrote:
> In preparation for being able to determine whether a given stack trace
> is reliable, allow the stacktrace_ops functions to propagate errors to
> dump_trace().

Hi, Josh.

Have you considered to make walk_stack function as non-return function,
since there is no obvious error during detecting the frame points?

Thanks
Minfei

Re: [PATCH] Use existing helper to convert "on/off" to boolean

[Minfei Huang]

On 04/29/16 at 10:04P, Michal Hocko wrote:
> On Fri 29-04-16 13:47:04, Minfei Huang wrote:
> > It's more convenient to use existing function helper to convert string
> > "on/off" to boolean.
> 
> But kstrtobool in linux-next only does "This routine returns 0 iff the
> first character is one of 'Yy1Nn0'" so it doesn't know about on/off.
> Or am I missing anything?

Hi, Michal.

Thanks for your reply.

Following is the kstrtobool comment from linus tree, which has explained
that this function can parse "on"/"off" string. Also Kees Cook has
posted such patch to fix this issue as well. So I think it's safe to fix
it.

"
  This routine returns 0 if the first character is one of 'Yy1Nn0', or
  [oO][NnFf] for "on" and "off". Otherwise it will return -EINVAL.  Value
  pointed to by res is updated upon finding a match.
"

  commit 4cc7ecb7f2a60e8deb783b8fbf7c1ae467acb920
  Author: Kees Cook <keesc...@chromium.org>
  Date:   Thu Mar 17 14:23:00 2016 -0700
  
  param: convert some "on"/"off" users to strtobool
  
  This changes several users of manual "on"/"off" parsing to use
  strtobool.
  
  Some side-effects:
  - these uses will now parse y/n/1/0 meaningfully too
  - the early_param uses will now bubble up parse errors

Thanks
Minfei

Re: [PATCH] Use existing helper to convert "on/off" to boolean

[Minfei Huang]

On 04/29/16 at 10:04P, Michal Hocko wrote:
> On Fri 29-04-16 13:47:04, Minfei Huang wrote:
> > It's more convenient to use existing function helper to convert string
> > "on/off" to boolean.
> 
> But kstrtobool in linux-next only does "This routine returns 0 iff the
> first character is one of 'Yy1Nn0'" so it doesn't know about on/off.
> Or am I missing anything?

Hi, Michal.

Thanks for your reply.

Following is the kstrtobool comment from linus tree, which has explained
that this function can parse "on"/"off" string. Also Kees Cook has
posted such patch to fix this issue as well. So I think it's safe to fix
it.

"
  This routine returns 0 if the first character is one of 'Yy1Nn0', or
  [oO][NnFf] for "on" and "off". Otherwise it will return -EINVAL.  Value
  pointed to by res is updated upon finding a match.
"

  commit 4cc7ecb7f2a60e8deb783b8fbf7c1ae467acb920
  Author: Kees Cook 
  Date:   Thu Mar 17 14:23:00 2016 -0700
  
  param: convert some "on"/"off" users to strtobool
  
  This changes several users of manual "on"/"off" parsing to use
  strtobool.
  
  Some side-effects:
  - these uses will now parse y/n/1/0 meaningfully too
  - the early_param uses will now bubble up parse errors

Thanks
Minfei

[PATCH] Use existing helper to convert "on/off" to boolean

[Minfei Huang]

It's more convenient to use existing function helper to convert string
"on/off" to boolean.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 lib/kstrtox.c| 2 +-
 mm/page_alloc.c  | 9 +
 mm/page_poison.c | 8 +---
 3 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index d8a5cf6..3c66fc4 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -326,7 +326,7 @@ EXPORT_SYMBOL(kstrtos8);
  * @s: input string
  * @res: result
  *
- * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
+ * This routine returns 0 if the first character is one of 'Yy1Nn0', or
  * [oO][NnFf] for "on" and "off". Otherwise it will return -EINVAL.  Value
  * pointed to by res is updated upon finding a match.
  */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 59de90d..d31426d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -513,14 +513,7 @@ static int __init early_debug_pagealloc(char *buf)
 {
if (!buf)
return -EINVAL;
-
-   if (strcmp(buf, "on") == 0)
-   _debug_pagealloc_enabled = true;
-
-   if (strcmp(buf, "off") == 0)
-   _debug_pagealloc_enabled = false;
-
-   return 0;
+   return kstrtobool(buf, &_debug_pagealloc_enabled);
 }
 early_param("debug_pagealloc", early_debug_pagealloc);
 
diff --git a/mm/page_poison.c b/mm/page_poison.c
index 479e7ea..1eae5fa 100644
--- a/mm/page_poison.c
+++ b/mm/page_poison.c
@@ -13,13 +13,7 @@ static int early_page_poison_param(char *buf)
 {
if (!buf)
return -EINVAL;
-
-   if (strcmp(buf, "on") == 0)
-   want_page_poisoning = true;
-   else if (strcmp(buf, "off") == 0)
-   want_page_poisoning = false;
-
-   return 0;
+   return strtobool(buf, _page_poisoning);
 }
 early_param("page_poison", early_page_poison_param);
 
-- 
2.6.3

[PATCH] Use existing helper to convert "on/off" to boolean

[Minfei Huang]

It's more convenient to use existing function helper to convert string
"on/off" to boolean.

Signed-off-by: Minfei Huang 
---
 lib/kstrtox.c| 2 +-
 mm/page_alloc.c  | 9 +
 mm/page_poison.c | 8 +---
 3 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index d8a5cf6..3c66fc4 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -326,7 +326,7 @@ EXPORT_SYMBOL(kstrtos8);
  * @s: input string
  * @res: result
  *
- * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
+ * This routine returns 0 if the first character is one of 'Yy1Nn0', or
  * [oO][NnFf] for "on" and "off". Otherwise it will return -EINVAL.  Value
  * pointed to by res is updated upon finding a match.
  */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 59de90d..d31426d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -513,14 +513,7 @@ static int __init early_debug_pagealloc(char *buf)
 {
if (!buf)
return -EINVAL;
-
-   if (strcmp(buf, "on") == 0)
-   _debug_pagealloc_enabled = true;
-
-   if (strcmp(buf, "off") == 0)
-   _debug_pagealloc_enabled = false;
-
-   return 0;
+   return kstrtobool(buf, &_debug_pagealloc_enabled);
 }
 early_param("debug_pagealloc", early_debug_pagealloc);
 
diff --git a/mm/page_poison.c b/mm/page_poison.c
index 479e7ea..1eae5fa 100644
--- a/mm/page_poison.c
+++ b/mm/page_poison.c
@@ -13,13 +13,7 @@ static int early_page_poison_param(char *buf)
 {
if (!buf)
return -EINVAL;
-
-   if (strcmp(buf, "on") == 0)
-   want_page_poisoning = true;
-   else if (strcmp(buf, "off") == 0)
-   want_page_poisoning = false;
-
-   return 0;
+   return strtobool(buf, _page_poisoning);
 }
 early_param("page_poison", early_page_poison_param);
 
-- 
2.6.3

Re: [PATCH] Use MICRO UINT_MAX instead of actual value

[Minfei Huang]

On 04/25/16 at 12:44P, Sagi Grimberg wrote:
> 
> >It's more elegant to use MICRO UINT_MAX to represent the max value of
> >type unsigned int. So replace the actual value by using this MICRO.
> 
> You mean macro right?

Yep. Sorry for the typo error.

Thanks
Minfei

Re: [PATCH] Use MICRO UINT_MAX instead of actual value

[Minfei Huang]

On 04/25/16 at 12:44P, Sagi Grimberg wrote:
> 
> >It's more elegant to use MICRO UINT_MAX to represent the max value of
> >type unsigned int. So replace the actual value by using this MICRO.
> 
> You mean macro right?

Yep. Sorry for the typo error.

Thanks
Minfei

[PATCH] Make clocksource insert entry more efficiently

[Minfei Huang]

It is unnecessary to continue looping the list, if we find there is an
entry that the value of rating is smaller than the new one. It is safe
to be out the loop, because all of entry are inserted in descending
order.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 kernel/time/clocksource.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 56ece14..6a5a310 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -669,10 +669,12 @@ static void clocksource_enqueue(struct clocksource *cs)
struct list_head *entry = _list;
struct clocksource *tmp;
 
-   list_for_each_entry(tmp, _list, list)
+   list_for_each_entry(tmp, _list, list) {
/* Keep track of the place, where to insert */
-   if (tmp->rating >= cs->rating)
-   entry = >list;
+   if (tmp->rating < cs->rating)
+   break;
+   entry = >list;
+   }
list_add(>list, entry);
 }
 
-- 
2.6.3

[PATCH] Make clocksource insert entry more efficiently

[Minfei Huang]

It is unnecessary to continue looping the list, if we find there is an
entry that the value of rating is smaller than the new one. It is safe
to be out the loop, because all of entry are inserted in descending
order.

Signed-off-by: Minfei Huang 
---
 kernel/time/clocksource.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 56ece14..6a5a310 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -669,10 +669,12 @@ static void clocksource_enqueue(struct clocksource *cs)
struct list_head *entry = _list;
struct clocksource *tmp;
 
-   list_for_each_entry(tmp, _list, list)
+   list_for_each_entry(tmp, _list, list) {
/* Keep track of the place, where to insert */
-   if (tmp->rating >= cs->rating)
-   entry = >list;
+   if (tmp->rating < cs->rating)
+   break;
+   entry = >list;
+   }
list_add(>list, entry);
 }
 
-- 
2.6.3

[PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Minfei Huang]

The value of cycles and flags can be assigned directly without
intermediate variables.

Remove the useless variables.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 arch/x86/include/asm/pvclock.h | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index fdcc040..fb95dac 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -80,19 +80,12 @@ static __always_inline
 unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
   cycle_t *cycles, u8 *flags)
 {
-   unsigned version;
-   cycle_t ret, offset;
-   u8 ret_flags;
-
-   version = src->version;
+   cycle_t offset;
 
offset = pvclock_get_nsec_offset(src);
-   ret = src->system_time + offset;
-   ret_flags = src->flags;
-
-   *cycles = ret;
-   *flags = ret_flags;
-   return version;
+   *cycles = src->system_time + offset;
+   *flags = src->flags;
+   return src->version;
 }
 
 struct pvclock_vsyscall_time_info {
-- 
2.6.3

[PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Minfei Huang]

The value of cycles and flags can be assigned directly without
intermediate variables.

Remove the useless variables.

Signed-off-by: Minfei Huang 
---
 arch/x86/include/asm/pvclock.h | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index fdcc040..fb95dac 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -80,19 +80,12 @@ static __always_inline
 unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
   cycle_t *cycles, u8 *flags)
 {
-   unsigned version;
-   cycle_t ret, offset;
-   u8 ret_flags;
-
-   version = src->version;
+   cycle_t offset;
 
offset = pvclock_get_nsec_offset(src);
-   ret = src->system_time + offset;
-   ret_flags = src->flags;
-
-   *cycles = ret;
-   *flags = ret_flags;
-   return version;
+   *cycles = src->system_time + offset;
+   *flags = src->flags;
+   return src->version;
 }
 
 struct pvclock_vsyscall_time_info {
-- 
2.6.3

[PATCH] Use MICRO UINT_MAX instead of actual value

[Minfei Huang]

It's more elegant to use MICRO UINT_MAX to represent the max value of
type unsigned int. So replace the actual value by using this MICRO.

Signed-off-by: Minfei Huang <mngh...@gmail.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 643f457..2c0bb13 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -597,7 +597,7 @@ static void nvme_config_discard(struct nvme_ns *ns)
 
ns->queue->limits.discard_alignment = logical_block_size;
ns->queue->limits.discard_granularity = logical_block_size;
-   blk_queue_max_discard_sectors(ns->queue, 0x);
+   blk_queue_max_discard_sectors(ns->queue, UINT_MAX);
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
 }
 
-- 
2.6.3

[PATCH] Use MICRO UINT_MAX instead of actual value

[Minfei Huang]

It's more elegant to use MICRO UINT_MAX to represent the max value of
type unsigned int. So replace the actual value by using this MICRO.

Signed-off-by: Minfei Huang 
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 643f457..2c0bb13 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -597,7 +597,7 @@ static void nvme_config_discard(struct nvme_ns *ns)
 
ns->queue->limits.discard_alignment = logical_block_size;
ns->queue->limits.discard_granularity = logical_block_size;
-   blk_queue_max_discard_sectors(ns->queue, 0x);
+   blk_queue_max_discard_sectors(ns->queue, UINT_MAX);
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, ns->queue);
 }
 
-- 
2.6.3