Message ID | b74dcb60-ef35-f06e-de2d-b165ed38036a@infradead.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [-next] mm/hugetlb.c: fix printk format warning for 32-bit phys_addr_t | expand |
On Wed, 2020-03-18 at 14:33 -0700, Randy Dunlap wrote: > From: Randy Dunlap <rdunlap@infradead.org> > > Fix printk format warnings when phys_addr_t is 32 bits, i.e., > CONFIG_PHYS_ADDR_T_64BIT is not set/enabled. [] > ../mm/hugetlb.c:5472:73: note: format string is defined here > pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%llu, %llu)", > ~~~^ [] > --- linux-next-20200318.orig/mm/hugetlb.c > +++ linux-next-20200318/mm/hugetlb.c > @@ -5469,8 +5469,10 @@ void __init hugetlb_cma_reserve(int orde > 0, false, > "hugetlb", &hugetlb_cma[nid]); > if (res) { > - pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%llu, %llu)", > - res, nid, PFN_PHYS(min_pfn), PFN_PHYS(max_pfn)); > + phys_addr_t begpa = PFN_PHYS(min_pfn); > + phys_addr_t endpa = PFN_PHYS(max_pfn); > + pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%pap, %pap)", > + res, nid, &begpa, &endpa); You might correct the odd use of an open bracket then close parenthesis and add a new line too Perhaps: pr_warn("%s: reservation failed: err %d, node %d, [%pap, %pap]\n", __func__, res, nid, &begpa, &endpa);
On 3/18/20 7:04 PM, Joe Perches wrote: > On Wed, 2020-03-18 at 14:33 -0700, Randy Dunlap wrote: >> From: Randy Dunlap <rdunlap@infradead.org> >> >> Fix printk format warnings when phys_addr_t is 32 bits, i.e., >> CONFIG_PHYS_ADDR_T_64BIT is not set/enabled. > [] >> ../mm/hugetlb.c:5472:73: note: format string is defined here >> pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%llu, %llu)", >> ~~~^ > [] >> --- linux-next-20200318.orig/mm/hugetlb.c >> +++ linux-next-20200318/mm/hugetlb.c >> @@ -5469,8 +5469,10 @@ void __init hugetlb_cma_reserve(int orde >> 0, false, >> "hugetlb", &hugetlb_cma[nid]); >> if (res) { >> - pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%llu, %llu)", >> - res, nid, PFN_PHYS(min_pfn), PFN_PHYS(max_pfn)); >> + phys_addr_t begpa = PFN_PHYS(min_pfn); >> + phys_addr_t endpa = PFN_PHYS(max_pfn); >> + pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%pap, %pap)", >> + res, nid, &begpa, &endpa); > > You might correct the odd use of an open bracket > then close parenthesis and add a new line too Definitely needs a newline char. I'm fairly sure that the [begin, end) notation is done on purpose, meaning <begin> is included in the range and <end> is not included in the range. > > Perhaps: > pr_warn("%s: reservation failed: err %d, node %d, [%pap, %pap]\n", > __func__, res, nid, &begpa, &endpa); > > thanks.
On Wed, 2020-03-18 at 19:11 -0700, Randy Dunlap wrote: > On 3/18/20 7:04 PM, Joe Perches wrote: > > On Wed, 2020-03-18 at 14:33 -0700, Randy Dunlap wrote: > > > From: Randy Dunlap <rdunlap@infradead.org> > > > > > > Fix printk format warnings when phys_addr_t is 32 bits, i.e., > > > CONFIG_PHYS_ADDR_T_64BIT is not set/enabled. > > [] > > > ../mm/hugetlb.c:5472:73: note: format string is defined here > > > pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%llu, %llu)", > > > ~~~^ > > [] > > > --- linux-next-20200318.orig/mm/hugetlb.c > > > +++ linux-next-20200318/mm/hugetlb.c > > > @@ -5469,8 +5469,10 @@ void __init hugetlb_cma_reserve(int orde > > > 0, false, > > > "hugetlb", &hugetlb_cma[nid]); > > > if (res) { > > > - pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%llu, %llu)", > > > - res, nid, PFN_PHYS(min_pfn), PFN_PHYS(max_pfn)); > > > + phys_addr_t begpa = PFN_PHYS(min_pfn); > > > + phys_addr_t endpa = PFN_PHYS(max_pfn); > > > + pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%pap, %pap)", > > > + res, nid, &begpa, &endpa); > > > > You might correct the odd use of an open bracket > > then close parenthesis and add a new line too > > Definitely needs a newline char. > > I'm fairly sure that the [begin, end) notation is done on purpose, meaning > <begin> is included in the range and <end> is not included in the range. OK, that seems a pretty obscure and not obvious use of interval notation, at least to me. (18 uses treewide ?) Maybe it could be documented somewhere? It's an odd pattern to grep. Maybe I missed some. $ git grep -P '".*[\[\{]\s*%\d*[ldux]+\s*[/:,\.\-]?\s*%\d*[ldux]+\).*"' arch/x86/kernel/alternative.c: DUMP_BYTES(instr, a->instrlen, "%px: [%d:%d) optimized NOPs: ", drivers/clk/qcom/clk-alpha-pll.c: pr_err("%s: Rounded rate %lu not within range [%lu, %lu)\n", fs/ext4/extents_status.c: printk(KERN_DEBUG " [%u/%u) %llu %x", fs/ext4/extents_status.c: es_debug("%u cached by [%u/%u) %llu %x\n", fs/ext4/extents_status.c: es_debug("add [%u/%u) %llu %x to extent status tree of inode %lu\n", fs/ext4/extents_status.c: es_debug("%u cached by [%u/%u)\n", fs/ext4/extents_status.c: es_debug("remove [%u/%u) from extent status tree of inode %lu\n", fs/nilfs2/cpfile.c: "cannot delete checkpoints: invalid range [%llu, %llu)", fs/nilfs2/dat.c: "%s: invalid vblocknr = %llu, [%llu, %llu)", include/trace/events/ext4.h: TP_printk("dev %d,%d ino %lu es [%u/%u) mapped %llu status %s", include/trace/events/ext4.h: TP_printk("dev %d,%d ino %lu es [%lld/%lld)", include/trace/events/ext4.h: TP_printk("dev %d,%d ino %lu es [%u/%u) mapped %llu status %s", include/trace/events/ext4.h: TP_printk("dev %d,%d ino %lu found %d [%u/%u) %llu %s", include/trace/events/ext4.h: TP_printk("dev %d,%d ino %lu es [%u/%u) mapped %llu status %s " mm/hugetlb.c: pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%llu, %llu)", mm/kasan/report.c: pr_err(" [%lu, %lu) '%s'", offset, offset + size, token); mm/page_alloc.c: pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n", tools/testing/selftests/kvm/demand_paging_test.c: PER_VCPU_DEBUG("Added VCPU %d with test mem gpa [%lx, %lx)\n",
On 3/18/20 7:37 PM, Joe Perches wrote: > On Wed, 2020-03-18 at 19:11 -0700, Randy Dunlap wrote: >> On 3/18/20 7:04 PM, Joe Perches wrote: >>> On Wed, 2020-03-18 at 14:33 -0700, Randy Dunlap wrote: >>>> From: Randy Dunlap <rdunlap@infradead.org> >>>> >>>> Fix printk format warnings when phys_addr_t is 32 bits, i.e., >>>> CONFIG_PHYS_ADDR_T_64BIT is not set/enabled. >>> [] >>>> ../mm/hugetlb.c:5472:73: note: format string is defined here >>>> pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%llu, %llu)", >>>> ~~~^ >>> [] >>>> --- linux-next-20200318.orig/mm/hugetlb.c >>>> +++ linux-next-20200318/mm/hugetlb.c >>>> @@ -5469,8 +5469,10 @@ void __init hugetlb_cma_reserve(int orde >>>> 0, false, >>>> "hugetlb", &hugetlb_cma[nid]); >>>> if (res) { >>>> - pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%llu, %llu)", >>>> - res, nid, PFN_PHYS(min_pfn), PFN_PHYS(max_pfn)); >>>> + phys_addr_t begpa = PFN_PHYS(min_pfn); >>>> + phys_addr_t endpa = PFN_PHYS(max_pfn); >>>> + pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%pap, %pap)", >>>> + res, nid, &begpa, &endpa); >>> >>> You might correct the odd use of an open bracket >>> then close parenthesis and add a new line too >> >> Definitely needs a newline char. >> >> I'm fairly sure that the [begin, end) notation is done on purpose, meaning >> <begin> is included in the range and <end> is not included in the range. > > OK, that seems a pretty obscure and not obvious use of > interval notation, at least to me. (18 uses treewide ?) > > Maybe it could be documented somewhere? I thought about where to put that and came up empty. > It's an odd pattern to grep. Maybe I missed some. It's probably not used much more than this. > $ git grep -P '".*[\[\{]\s*%\d*[ldux]+\s*[/:,\.\-]?\s*%\d*[ldux]+\).*"' > arch/x86/kernel/alternative.c: DUMP_BYTES(instr, a->instrlen, "%px: [%d:%d) optimized NOPs: ", > drivers/clk/qcom/clk-alpha-pll.c: pr_err("%s: Rounded rate %lu not within range [%lu, %lu)\n", > fs/ext4/extents_status.c: printk(KERN_DEBUG " [%u/%u) %llu %x", > fs/ext4/extents_status.c: es_debug("%u cached by [%u/%u) %llu %x\n", > fs/ext4/extents_status.c: es_debug("add [%u/%u) %llu %x to extent status tree of inode %lu\n", > fs/ext4/extents_status.c: es_debug("%u cached by [%u/%u)\n", > fs/ext4/extents_status.c: es_debug("remove [%u/%u) from extent status tree of inode %lu\n", > fs/nilfs2/cpfile.c: "cannot delete checkpoints: invalid range [%llu, %llu)", > fs/nilfs2/dat.c: "%s: invalid vblocknr = %llu, [%llu, %llu)", > include/trace/events/ext4.h: TP_printk("dev %d,%d ino %lu es [%u/%u) mapped %llu status %s", > include/trace/events/ext4.h: TP_printk("dev %d,%d ino %lu es [%lld/%lld)", > include/trace/events/ext4.h: TP_printk("dev %d,%d ino %lu es [%u/%u) mapped %llu status %s", > include/trace/events/ext4.h: TP_printk("dev %d,%d ino %lu found %d [%u/%u) %llu %s", > include/trace/events/ext4.h: TP_printk("dev %d,%d ino %lu es [%u/%u) mapped %llu status %s " > mm/hugetlb.c: pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%llu, %llu)", > mm/kasan/report.c: pr_err(" [%lu, %lu) '%s'", offset, offset + size, token); > mm/page_alloc.c: pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n", > tools/testing/selftests/kvm/demand_paging_test.c: PER_VCPU_DEBUG("Added VCPU %d with test mem gpa [%lx, %lx)\n", > > thanks.
On Thu, 2020-03-19 at 18:03 -0700, Randy Dunlap wrote: > On 3/18/20 7:37 PM, Joe Perches wrote: > > On Wed, 2020-03-18 at 19:11 -0700, Randy Dunlap wrote: > > > I'm fairly sure that the [begin, end) notation is done on purpose, meaning > > > > > > <begin> is included in the range and <end> is not included in the range. > > > > OK, that seems a pretty obscure and not obvious use of > > interval notation, at least to me. (18 uses treewide ?) > > > > Maybe it could be documented somewhere? > > I thought about where to put that and came up empty. No worries then, it's likely not _too_ obscure to anyone that's sent a log for analysis.
--- linux-next-20200318.orig/mm/hugetlb.c +++ linux-next-20200318/mm/hugetlb.c @@ -5469,8 +5469,10 @@ void __init hugetlb_cma_reserve(int orde 0, false, "hugetlb", &hugetlb_cma[nid]); if (res) { - pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%llu, %llu)", - res, nid, PFN_PHYS(min_pfn), PFN_PHYS(max_pfn)); + phys_addr_t begpa = PFN_PHYS(min_pfn); + phys_addr_t endpa = PFN_PHYS(max_pfn); + pr_warn("hugetlb_cma: reservation failed: err %d, node %d, [%pap, %pap)", + res, nid, &begpa, &endpa); break; }