Hello,
syzbot found the following issue on:
HEAD commit: 44c026a73be8 Linux 6.4-rc3
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13a92b31280000
kernel config: https://syzkaller.appspot.com/x/.config?x=f389ffdf4e9ba3f0
dashboard link: https://syzkaller.appspot.com/bug?extid=841a46899768ec7bec67
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
userspace arch: i386
Unfortunately, I don't have any reproducer for this issue yet.
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/35f16ee05df7/disk-44c026a7.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/10399498a570/vmlinux-44c026a7.xz
kernel image: https://storage.googleapis.com/syzbot-assets/5c72201ea4ba/bzImage-44c026a7.xz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: [email protected]
divide error: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 13527 Comm: syz-executor.1 Not tainted 6.4.0-rc3-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/28/2023
RIP: 0010:damon_nr_accesses_to_accesses_bp mm/damon/core.c:491 [inline]
RIP: 0010:damon_nr_accesses_for_new_attrs mm/damon/core.c:497 [inline]
RIP: 0010:damon_update_monitoring_result mm/damon/core.c:506 [inline]
RIP: 0010:damon_update_monitoring_results mm/damon/core.c:534 [inline]
RIP: 0010:damon_set_attrs+0x224/0x460 mm/damon/core.c:555
Code: 03 42 0f b6 04 20 84 c0 74 08 3c 03 0f 8e b3 01 00 00 48 8b 43 08 31 d2 41 69 4f 18 10 27 00 00 48 f7 33 31 d2 48 89 c7 89 c8 <f7> f7 31 d2 49 8d 7f 30 89 c1 49 8b 46 08 49 f7 36 0f af c8 b8 59
RSP: 0018:ffffc90011627be8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff888015a88800 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff81e2f0e0 RDI: 0000000000000000
RBP: ffff88802b5ae710 R08: 0000000000000007 R09: 0000000000000000
R10: 0000000000000008 R11: 0000000000000000 R12: dffffc0000000000
R13: ffff88802b5ae700 R14: ffffc90011627c60 R15: ffff88802ef03058
FS: 0000000000000000(0000) GS:ffff8880b9900000(0063) knlGS:00000000f7f1db40
CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
CR2: 00000000f72b2f2b CR3: 00000000295e7000 CR4: 00000000003506e0
DR0: 00000000000017b9 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
dbgfs_attrs_write+0x225/0x280 mm/damon/dbgfs.c:101
full_proxy_write+0x11c/0x190 fs/debugfs/file.c:236
vfs_write+0x2ae/0xd50 fs/read_write.c:582
ksys_write+0x12b/0x250 fs/read_write.c:637
do_syscall_32_irqs_on arch/x86/entry/common.c:112 [inline]
__do_fast_syscall_32+0x65/0xf0 arch/x86/entry/common.c:178
do_fast_syscall_32+0x33/0x70 arch/x86/entry/common.c:203
entry_SYSENTER_compat_after_hwframe+0x70/0x82
RIP: 0023:0xf7f22579
Code: b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 00 00 00 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d b4 26 00 00 00 00 8d b4 26 00 00 00 00
RSP: 002b:00000000f7f1d5cc EFLAGS: 00000296 ORIG_RAX: 0000000000000004
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000020000040
RDX: 0000000000000069 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
</TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:damon_nr_accesses_to_accesses_bp mm/damon/core.c:491 [inline]
RIP: 0010:damon_nr_accesses_for_new_attrs mm/damon/core.c:497 [inline]
RIP: 0010:damon_update_monitoring_result mm/damon/core.c:506 [inline]
RIP: 0010:damon_update_monitoring_results mm/damon/core.c:534 [inline]
RIP: 0010:damon_set_attrs+0x224/0x460 mm/damon/core.c:555
Code: 03 42 0f b6 04 20 84 c0 74 08 3c 03 0f 8e b3 01 00 00 48 8b 43 08 31 d2 41 69 4f 18 10 27 00 00 48 f7 33 31 d2 48 89 c7 89 c8 <f7> f7 31 d2 49 8d 7f 30 89 c1 49 8b 46 08 49 f7 36 0f af c8 b8 59
RSP: 0018:ffffc90011627be8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff888015a88800 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff81e2f0e0 RDI: 0000000000000000
RBP: ffff88802b5ae710 R08: 0000000000000007 R09: 0000000000000000
R10: 0000000000000008 R11: 0000000000000000 R12: dffffc0000000000
R13: ffff88802b5ae700 R14: ffffc90011627c60 R15: ffff88802ef03058
FS: 0000000000000000(0000) GS:ffff8880b9800000(0063) knlGS:00000000f7f1db40
CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
CR2: 000000002cc30000 CR3: 00000000295e7000 CR4: 00000000003526f0
DR0: 00000000000017b9 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
----------------
Code disassembly (best guess):
0: 03 42 0f add 0xf(%rdx),%eax
3: b6 04 mov $0x4,%dh
5: 20 84 c0 74 08 3c 03 and %al,0x33c0874(%rax,%rax,8)
c: 0f 8e b3 01 00 00 jle 0x1c5
12: 48 8b 43 08 mov 0x8(%rbx),%rax
16: 31 d2 xor %edx,%edx
18: 41 69 4f 18 10 27 00 imul $0x2710,0x18(%r15),%ecx
1f: 00
20: 48 f7 33 divq (%rbx)
23: 31 d2 xor %edx,%edx
25: 48 89 c7 mov %rax,%rdi
28: 89 c8 mov %ecx,%eax
* 2a: f7 f7 div %edi <-- trapping instruction
2c: 31 d2 xor %edx,%edx
2e: 49 8d 7f 30 lea 0x30(%r15),%rdi
32: 89 c1 mov %eax,%ecx
34: 49 8b 46 08 mov 0x8(%r14),%rax
38: 49 f7 36 divq (%r14)
3b: 0f af c8 imul %eax,%ecx
3e: b8 .byte 0xb8
3f: 59 pop %rcx
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at [email protected].
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title
If you want to change bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)
If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report
If you want to undo deduplication, reply with:
#syz undup
On 2023/5/26 19:51, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 44c026a73be8 Linux 6.4-rc3
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=13a92b31280000
> kernel config: https://syzkaller.appspot.com/x/.config?x=f389ffdf4e9ba3f0
> dashboard link: https://syzkaller.appspot.com/bug?extid=841a46899768ec7bec67
> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> userspace arch: i386
>
> Unfortunately, I don't have any reproducer for this issue yet.
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/35f16ee05df7/disk-44c026a7.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/10399498a570/vmlinux-44c026a7.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/5c72201ea4ba/bzImage-44c026a7.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: [email protected]
>
> divide error: 0000 [#1] PREEMPT SMP KASAN
> CPU: 1 PID: 13527 Comm: syz-executor.1 Not tainted 6.4.0-rc3-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/28/2023
> RIP: 0010:damon_nr_accesses_to_accesses_bp mm/damon/core.c:491 [inline]
> RIP: 0010:damon_nr_accesses_for_new_attrs mm/damon/core.c:497 [inline]
> RIP: 0010:damon_update_monitoring_result mm/damon/core.c:506 [inline]
> RIP: 0010:damon_update_monitoring_results mm/damon/core.c:534 [inline]
> RIP: 0010:damon_set_attrs+0x224/0x460 mm/damon/core.c:555
make aggr_interval great than or equal sample_interval?
diff --git a/mm/damon/core.c b/mm/damon/core.c
index d9ef62047bf5..6fe1960f3d6b 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -525,8 +525,8 @@ static void damon_update_monitoring_results(struct
damon_ctx *ctx,
/* if any interval is zero, simply forgive conversion */
if (!old_attrs->sample_interval || !old_attrs->aggr_interval ||
- !new_attrs->sample_interval ||
- !new_attrs->aggr_interval)
+ !new_attrs->sample_interval || !new_attrs->aggr_interval ||
+ new_attrs->aggr_interval < new_attrs->sample_interval)
return;
Hi Kefeng and syzbot,
On Fri, 26 May 2023 20:59:12 +0800 Kefeng Wang <[email protected]> wrote:
>
>
> On 2023/5/26 19:51, syzbot wrote:
> > Hello,
> >
> > syzbot found the following issue on:
> >
> > HEAD commit: 44c026a73be8 Linux 6.4-rc3
> > git tree: upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=13a92b31280000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=f389ffdf4e9ba3f0
> > dashboard link: https://syzkaller.appspot.com/bug?extid=841a46899768ec7bec67
> > compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> > userspace arch: i386
> >
> > Unfortunately, I don't have any reproducer for this issue yet.
> >
> > Downloadable assets:
> > disk image: https://storage.googleapis.com/syzbot-assets/35f16ee05df7/disk-44c026a7.raw.xz
> > vmlinux: https://storage.googleapis.com/syzbot-assets/10399498a570/vmlinux-44c026a7.xz
> > kernel image: https://storage.googleapis.com/syzbot-assets/5c72201ea4ba/bzImage-44c026a7.xz
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: [email protected]
> >
> > divide error: 0000 [#1] PREEMPT SMP KASAN
> > CPU: 1 PID: 13527 Comm: syz-executor.1 Not tainted 6.4.0-rc3-syzkaller #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/28/2023
> > RIP: 0010:damon_nr_accesses_to_accesses_bp mm/damon/core.c:491 [inline]
> > RIP: 0010:damon_nr_accesses_for_new_attrs mm/damon/core.c:497 [inline]
> > RIP: 0010:damon_update_monitoring_result mm/damon/core.c:506 [inline]
> > RIP: 0010:damon_update_monitoring_results mm/damon/core.c:534 [inline]
> > RIP: 0010:damon_set_attrs+0x224/0x460 mm/damon/core.c:555
Thank you for finding and reporting this bug!
The code of the problem is as below:
/* convert nr_accesses to access ratio in bp (per 10,000) */
static unsigned int damon_nr_accesses_to_accesses_bp(
unsigned int nr_accesses, struct damon_attrs *attrs)
{
unsigned int max_nr_accesses =
attrs->aggr_interval / attrs->sample_interval;
return nr_accesses * 10000 / max_nr_accesses;
}
The problem can happen when 'aggr_interval' is smaller than 'sample_interval',
because 'max_nr_accesses' becomes zero in the case, and resulting in divide by
zero.
Same problem is in damon_accesses_bp_to_nr_accesses().
>
> make aggr_interval great than or equal sample_interval?
>
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index d9ef62047bf5..6fe1960f3d6b 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -525,8 +525,8 @@ static void damon_update_monitoring_results(struct
> damon_ctx *ctx,
>
> /* if any interval is zero, simply forgive conversion */
> if (!old_attrs->sample_interval || !old_attrs->aggr_interval ||
> - !new_attrs->sample_interval ||
> - !new_attrs->aggr_interval)
> + !new_attrs->sample_interval || !new_attrs->aggr_interval ||
> + new_attrs->aggr_interval < new_attrs->sample_interval)
> return;
Nice and effective fix! Nevertheless, I think aggregation interval smaller
than sample interval is just a wrong input. How about adding the check in
damon_set_attrs()'s already existing attributes validation, like below?
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -580,6 +580,8 @@ int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs)
return -EINVAL;
if (attrs->min_nr_regions > attrs->max_nr_regions)
return -EINVAL;
+ if (attrs->aggr_interval < attrs->sample_interval)
+ return -EINVAL;
damon_update_monitoring_results(ctx, attrs);
ctx->attrs = *attrs;
Thanks,
SJ
On Fri, 26 May 2023 18:54:09 +0000 SeongJae Park <[email protected]> wrote:
[...]
> > >
> > > divide error: 0000 [#1] PREEMPT SMP KASAN
> > > CPU: 1 PID: 13527 Comm: syz-executor.1 Not tainted 6.4.0-rc3-syzkaller #0
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/28/2023
> > > RIP: 0010:damon_nr_accesses_to_accesses_bp mm/damon/core.c:491 [inline]
> > > RIP: 0010:damon_nr_accesses_for_new_attrs mm/damon/core.c:497 [inline]
> > > RIP: 0010:damon_update_monitoring_result mm/damon/core.c:506 [inline]
> > > RIP: 0010:damon_update_monitoring_results mm/damon/core.c:534 [inline]
> > > RIP: 0010:damon_set_attrs+0x224/0x460 mm/damon/core.c:555
>
> Thank you for finding and reporting this bug!
>
> The code of the problem is as below:
>
> /* convert nr_accesses to access ratio in bp (per 10,000) */
> static unsigned int damon_nr_accesses_to_accesses_bp(
> unsigned int nr_accesses, struct damon_attrs *attrs)
> {
> unsigned int max_nr_accesses =
> attrs->aggr_interval / attrs->sample_interval;
>
> return nr_accesses * 10000 / max_nr_accesses;
> }
>
> The problem can happen when 'aggr_interval' is smaller than 'sample_interval',
> because 'max_nr_accesses' becomes zero in the case, and resulting in divide by
> zero.
>
> Same problem is in damon_accesses_bp_to_nr_accesses().
This is not true, because the function doesn't divide something by
'max_nr_acceses'. I was confused, sorry.
Thanks,
SJ
[...]
On 2023/5/27 2:54, SeongJae Park wrote:
> Hi Kefeng and syzbot,
>
> On Fri, 26 May 2023 20:59:12 +0800 Kefeng Wang <[email protected]> wrote:
>
>>
>>
>> On 2023/5/26 19:51, syzbot wrote:
>>> Hello,
>>>
>>> syzbot found the following issue on:
>>>
>>> HEAD commit: 44c026a73be8 Linux 6.4-rc3
>>> git tree: upstream
>>> console output: https://syzkaller.appspot.com/x/log.txt?x=13a92b31280000
>>> kernel config: https://syzkaller.appspot.com/x/.config?x=f389ffdf4e9ba3f0
>>> dashboard link: https://syzkaller.appspot.com/bug?extid=841a46899768ec7bec67
>>> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
>>> userspace arch: i386
>>>
>>> Unfortunately, I don't have any reproducer for this issue yet.
>>>
>>> Downloadable assets:
>>> disk image: https://storage.googleapis.com/syzbot-assets/35f16ee05df7/disk-44c026a7.raw.xz
>>> vmlinux: https://storage.googleapis.com/syzbot-assets/10399498a570/vmlinux-44c026a7.xz
>>> kernel image: https://storage.googleapis.com/syzbot-assets/5c72201ea4ba/bzImage-44c026a7.xz
>>>
>>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
>>> Reported-by: [email protected]
>>>
>>> divide error: 0000 [#1] PREEMPT SMP KASAN
>>> CPU: 1 PID: 13527 Comm: syz-executor.1 Not tainted 6.4.0-rc3-syzkaller #0
>>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/28/2023
>>> RIP: 0010:damon_nr_accesses_to_accesses_bp mm/damon/core.c:491 [inline]
>>> RIP: 0010:damon_nr_accesses_for_new_attrs mm/damon/core.c:497 [inline]
>>> RIP: 0010:damon_update_monitoring_result mm/damon/core.c:506 [inline]
>>> RIP: 0010:damon_update_monitoring_results mm/damon/core.c:534 [inline]
>>> RIP: 0010:damon_set_attrs+0x224/0x460 mm/damon/core.c:555
>
> Thank you for finding and reporting this bug!
>
> The code of the problem is as below:
>
> /* convert nr_accesses to access ratio in bp (per 10,000) */
> static unsigned int damon_nr_accesses_to_accesses_bp(
> unsigned int nr_accesses, struct damon_attrs *attrs)
> {
> unsigned int max_nr_accesses =
> attrs->aggr_interval / attrs->sample_interval;
>
> return nr_accesses * 10000 / max_nr_accesses;
> }
>
> The problem can happen when 'aggr_interval' is smaller than 'sample_interval',
> because 'max_nr_accesses' becomes zero in the case, and resulting in divide by
> zero.
>
> Same problem is in damon_accesses_bp_to_nr_accesses().
>
>>
>> make aggr_interval great than or equal sample_interval?
>>
>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>> index d9ef62047bf5..6fe1960f3d6b 100644
>> --- a/mm/damon/core.c
>> +++ b/mm/damon/core.c
>> @@ -525,8 +525,8 @@ static void damon_update_monitoring_results(struct
>> damon_ctx *ctx,
>>
>> /* if any interval is zero, simply forgive conversion */
>> if (!old_attrs->sample_interval || !old_attrs->aggr_interval ||
>> - !new_attrs->sample_interval ||
>> - !new_attrs->aggr_interval)
>> + !new_attrs->sample_interval || !new_attrs->aggr_interval ||
>> + new_attrs->aggr_interval < new_attrs->sample_interval)
>> return;
>
> Nice and effective fix! Nevertheless, I think aggregation interval smaller
> than sample interval is just a wrong input. How about adding the check in
> damon_set_attrs()'s already existing attributes validation, like below?
Yes, move the check into damon_set_attrs() is better, and it seems that
we could move all the check into it, and drop the old_attrs check in
damon_update_monitoring_results(), what's you option?
diff --git a/mm/damon/core.c b/mm/damon/core.c
index d9ef62047bf5..1647f7f1f708 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -523,12 +523,6 @@ static void damon_update_monitoring_results(struct
damon_ctx *ctx,
struct damon_target *t;
struct damon_region *r;
- /* if any interval is zero, simply forgive conversion */
- if (!old_attrs->sample_interval || !old_attrs->aggr_interval ||
- !new_attrs->sample_interval ||
- !new_attrs->aggr_interval)
- return;
-
damon_for_each_target(t, ctx)
damon_for_each_region(r, t)
damon_update_monitoring_result(
@@ -551,6 +545,10 @@ int damon_set_attrs(struct damon_ctx *ctx, struct
damon_attrs *attrs)
return -EINVAL;
if (attrs->min_nr_regions > attrs->max_nr_regions)
return -EINVAL;
+ if (attrs->sample_interval > attrs->aggr_interval)
+ return -EINVAL;
+ if (!attrs->sample_interval || !attrs->aggr_interval)
+ return -EINVAL;
>
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -580,6 +580,8 @@ int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs)
> return -EINVAL;
> if (attrs->min_nr_regions > attrs->max_nr_regions)
> return -EINVAL;
> + if (attrs->aggr_interval < attrs->sample_interval)
> + return -EINVAL;
>
> damon_update_monitoring_results(ctx, attrs);
> ctx->attrs = *attrs;
>
> Thanks,
> SJ
Hi Kefeng,
On Sat, 27 May 2023 09:15:01 +0800 Kefeng Wang <[email protected]> wrote:
[...]
> >
> > Nice and effective fix! Nevertheless, I think aggregation interval smaller
> > than sample interval is just a wrong input. How about adding the check in
> > damon_set_attrs()'s already existing attributes validation, like below?
>
> Yes, move the check into damon_set_attrs() is better
Thank you for this kind comment!
> , and it seems that
> we could move all the check into it, and drop the old_attrs check in
> damon_update_monitoring_results(), what's you option?
>
>
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index d9ef62047bf5..1647f7f1f708 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -523,12 +523,6 @@ static void damon_update_monitoring_results(struct
> damon_ctx *ctx,
> struct damon_target *t;
> struct damon_region *r;
>
> - /* if any interval is zero, simply forgive conversion */
> - if (!old_attrs->sample_interval || !old_attrs->aggr_interval ||
> - !new_attrs->sample_interval ||
> - !new_attrs->aggr_interval)
> - return;
> -
> damon_for_each_target(t, ctx)
> damon_for_each_region(r, t)
> damon_update_monitoring_result(
> @@ -551,6 +545,10 @@ int damon_set_attrs(struct damon_ctx *ctx, struct
> damon_attrs *attrs)
> return -EINVAL;
> if (attrs->min_nr_regions > attrs->max_nr_regions)
> return -EINVAL;
> + if (attrs->sample_interval > attrs->aggr_interval)
> + return -EINVAL;
> + if (!attrs->sample_interval || !attrs->aggr_interval)
> + return -EINVAL;
In my humble opinion, the validation for monitoring results and for general
monitoring could be different. For example, zero aggreation/sampling intervals
might make sense for fixed granularity working set size monitoring. Hence, I'd
prefer keeping those checks in the damon_update_monitoring_results().
Thanks,
SJ
[...]
On 2023/5/27 9:46, SeongJae Park wrote:
> Hi Kefeng,
>
> On Sat, 27 May 2023 09:15:01 +0800 Kefeng Wang <[email protected]> wrote:
>
> [...]
>>>
>>> Nice and effective fix! Nevertheless, I think aggregation interval smaller
>>> than sample interval is just a wrong input. How about adding the check in
>>> damon_set_attrs()'s already existing attributes validation, like below?
>>
>> Yes, move the check into damon_set_attrs() is better
>
> Thank you for this kind comment!
>
>> , and it seems that
>> we could move all the check into it, and drop the old_attrs check in
>> damon_update_monitoring_results(), what's you option?
>>
>>
>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>> index d9ef62047bf5..1647f7f1f708 100644
>> --- a/mm/damon/core.c
>> +++ b/mm/damon/core.c
>> @@ -523,12 +523,6 @@ static void damon_update_monitoring_results(struct
>> damon_ctx *ctx,
>> struct damon_target *t;
>> struct damon_region *r;
>>
>> - /* if any interval is zero, simply forgive conversion */
>> - if (!old_attrs->sample_interval || !old_attrs->aggr_interval ||
>> - !new_attrs->sample_interval ||
>> - !new_attrs->aggr_interval)
>> - return;
>> -
>> damon_for_each_target(t, ctx)
>> damon_for_each_region(r, t)
>> damon_update_monitoring_result(
>> @@ -551,6 +545,10 @@ int damon_set_attrs(struct damon_ctx *ctx, struct
>> damon_attrs *attrs)
>> return -EINVAL;
>> if (attrs->min_nr_regions > attrs->max_nr_regions)
>> return -EINVAL;
>> + if (attrs->sample_interval > attrs->aggr_interval)
>> + return -EINVAL;
>> + if (!attrs->sample_interval || !attrs->aggr_interval)
>> + return -EINVAL;
>
> In my humble opinion, the validation for monitoring results and for general
> monitoring could be different. For example, zero aggreation/sampling intervals
> might make sense for fixed granularity working set size monitoring. Hence, I'd
> prefer keeping those checks in the damon_update_monitoring_results().
ok, will keep that, I check the damon_set_attrs() called by
lru_sort/reclaim monitor and sysfs/dbgfs, the above changes should be
ok, maybe missing something, the working set size monitoring is not
public for now?
>
>
> Thanks,
> SJ
>
> [...]
On Sat, 27 May 2023 10:02:38 +0800 Kefeng Wang <[email protected]> wrote:
>
>
> On 2023/5/27 9:46, SeongJae Park wrote:
> > Hi Kefeng,
> >
> > On Sat, 27 May 2023 09:15:01 +0800 Kefeng Wang <[email protected]> wrote:
> >
> > [...]
> >>>
> >>> Nice and effective fix! Nevertheless, I think aggregation interval smaller
> >>> than sample interval is just a wrong input. How about adding the check in
> >>> damon_set_attrs()'s already existing attributes validation, like below?
> >>
> >> Yes, move the check into damon_set_attrs() is better
> >
> > Thank you for this kind comment!
> >
> >> , and it seems that
> >> we could move all the check into it, and drop the old_attrs check in
> >> damon_update_monitoring_results(), what's you option?
> >>
> >>
> >> diff --git a/mm/damon/core.c b/mm/damon/core.c
> >> index d9ef62047bf5..1647f7f1f708 100644
> >> --- a/mm/damon/core.c
> >> +++ b/mm/damon/core.c
> >> @@ -523,12 +523,6 @@ static void damon_update_monitoring_results(struct
> >> damon_ctx *ctx,
> >> struct damon_target *t;
> >> struct damon_region *r;
> >>
> >> - /* if any interval is zero, simply forgive conversion */
> >> - if (!old_attrs->sample_interval || !old_attrs->aggr_interval ||
> >> - !new_attrs->sample_interval ||
> >> - !new_attrs->aggr_interval)
> >> - return;
> >> -
> >> damon_for_each_target(t, ctx)
> >> damon_for_each_region(r, t)
> >> damon_update_monitoring_result(
> >> @@ -551,6 +545,10 @@ int damon_set_attrs(struct damon_ctx *ctx, struct
> >> damon_attrs *attrs)
> >> return -EINVAL;
> >> if (attrs->min_nr_regions > attrs->max_nr_regions)
> >> return -EINVAL;
> >> + if (attrs->sample_interval > attrs->aggr_interval)
> >> + return -EINVAL;
> >> + if (!attrs->sample_interval || !attrs->aggr_interval)
> >> + return -EINVAL;
> >
> > In my humble opinion, the validation for monitoring results and for general
> > monitoring could be different. For example, zero aggreation/sampling intervals
> > might make sense for fixed granularity working set size monitoring. Hence, I'd
> > prefer keeping those checks in the damon_update_monitoring_results().
>
>
> ok, will keep that,
Thank you for agreeing.
> I check the damon_set_attrs() called by
> lru_sort/reclaim monitor and sysfs/dbgfs, the above changes should be
> ok, maybe missing something, the working set size monitoring is not
> public for now?
You're correct. Working set size monitoring is not somewhat currently
publicly exists, but only possible usage of DAMON at the moment.
Thanks,
SJ
[...]