[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] x86/mm: Drop MEM_LOG() and correct some printed information
On 29/03/17 14:06, Jan Beulich wrote: >>>> On 29.03.17 at 14:29, <andrew.cooper3@xxxxxxxxxx> wrote: >> * Use 0x prefix for otherwise unqualified hex numbers. > I'm glad this in fact refers to just a single place. > >> @@ -1057,10 +1062,10 @@ get_page_from_l1e( >> put_page_type(page); >> put_page(page); >> >> - MEM_LOG("Error updating mappings for mfn %lx (pfn %lx," >> - " from L1 entry %" PRIpte ") for %d", >> - mfn, get_gpfn_from_mfn(mfn), >> - l1e_get_intpte(l1e), l1e_owner->domain_id); >> + gdprintk(XENLOG_WARNING, "Error updating mappings for mfn %" >> PRI_mfn >> + " (pfn %" PRI_pfn ", from L1 entry %" PRIpte ") for >> d%d\n", >> + mfn, get_gpfn_from_mfn(mfn), >> + l1e_get_intpte(l1e), l1e_owner->domain_id); >> return err; >> } >> } >> @@ -1068,10 +1073,10 @@ get_page_from_l1e( >> return 0; >> >> could_not_pin: >> - MEM_LOG("Error getting mfn %lx (pfn %lx) from L1 entry %" PRIpte >> - " for l1e_owner=%d, pg_owner=%d", >> - mfn, get_gpfn_from_mfn(mfn), >> - l1e_get_intpte(l1e), l1e_owner->domain_id, pg_owner->domain_id); >> + gdprintk(XENLOG_WARNING, "Error getting mfn %" PRI_mfn " (pfn %" PRI_pfn >> + ") from L1 entry %" PRIpte " for l1e_owner d%d, pg_owner d%d", >> + mfn, get_gpfn_from_mfn(mfn), >> + l1e_get_intpte(l1e), l1e_owner->domain_id, >> pg_owner->domain_id); > Especially here the wrapping of the format string is rather > unfortunate. Didn't we agree to allow format strings to exceed > the 80 column restriction anyway? It is split at a formatting boundary, which doesn't affect grep-ability. Putting this all on one line is 123 characters, which IMO is too long. > >> @@ -1388,7 +1398,7 @@ static int alloc_l1_table(struct page_info *page) >> return 0; >> >> fail: >> - MEM_LOG("Failure in alloc_l1_table: entry %d", i); >> + gdprintk(XENLOG_WARNING, "Failure in alloc_l1_table: entry %d\n", i); > %u (or even %03x; same in alloc_l[234]_table()) Actually, "slot %#x" would be clearer here. I though I fixed the 0x prefix in alloc_l[]_table(), and I am not sure the leading zeroes are helpful. > >> @@ -1979,7 +1991,8 @@ static int mod_l2_entry(l2_pgentry_t *pl2e, >> >> if ( unlikely(!is_guest_l2_slot(d, type, pgentry_ptr_to_slot(pl2e))) ) >> { >> - MEM_LOG("Illegal L2 update attempt in Xen-private area %p", pl2e); >> + gdprintk(XENLOG_WARNING, >> + "Illegal L2 update attempt in Xen-private area %p\n", >> pl2e); > Could you make this message useful at once? The pointer is > not really helpful to diagnose anything, I think. Same for > mod_l[34]_entry() then. Yes - can switch them to slot information. > >> @@ -3179,7 +3208,7 @@ long do_mmuext_op( >> >> if ( unlikely(__copy_from_guest(&op, uops, 1) != 0) ) >> { >> - MEM_LOG("Bad __copy_from_guest"); >> + gdprintk(XENLOG_WARNING, "Bad __copy_from_guest\n"); > I'd suggest to drop this one altogether. Yeah - I had considered that. Will drop. > >> @@ -3195,7 +3224,8 @@ long do_mmuext_op( >> case MMUEXT_UNPIN_TABLE: >> break; >> default: >> - MEM_LOG("Invalid extended pt command %#x", op.cmd); >> + gdprintk(XENLOG_WARNING, >> + "Invalid extended pt command %#x\n", op.cmd); > And this one too. Ok. > >> @@ -3297,7 +3329,8 @@ long do_mmuext_op( >> page = get_page_from_gfn(pg_owner, op.arg1.mfn, NULL, >> P2M_ALLOC); >> if ( unlikely(!page) ) >> { >> - MEM_LOG("Mfn %lx bad domain", op.arg1.mfn); >> + gdprintk(XENLOG_WARNING, >> + "mfn %" PRI_mfn " bad domain\n", op.arg1.mfn); > Perhaps also include the domain which was supposedly bad? It is not that simple. This error message covers both the mfn being bad, and a good mfn not belonging to the requested domain. I will see if I can word something appropriately. > >> @@ -3458,7 +3493,8 @@ long do_mmuext_op( >> rc = -EPERM; >> else if ( unlikely(!cache_flush_permitted(d)) ) >> { >> - MEM_LOG("Non-physdev domain tried to FLUSH_CACHE."); >> + gdprintk(XENLOG_WARNING, >> + "Non-physdev domain tried to FLUSH_CACHE\n"); >> rc = -EACCES; >> } >> else >> @@ -3484,7 +3520,8 @@ long do_mmuext_op( >> } >> else >> { >> - MEM_LOG("Non-physdev domain tried to FLUSH_CACHE_GLOBAL"); >> + gdprintk(XENLOG_WARNING, >> + "Non-physdev domain tried to >> FLUSH_CACHE_GLOBAL\n"); >> rc = -EINVAL; >> } >> break; > I think these could also be dropped (and perhaps a few more right > below here). You presumably mean all the trivial ones returning -EINVAL. I will drop those as well. > >> @@ -3734,7 +3779,7 @@ long do_mmu_update( >> >> if ( unlikely(__copy_from_guest(&req, ureqs, 1) != 0) ) >> { >> - MEM_LOG("Bad __copy_from_guest"); >> + gdprintk(XENLOG_WARNING, "Bad __copy_from_guest\n"); > And this one. Ok. > >> @@ -4201,7 +4250,8 @@ static int replace_grant_va_mapping( >> /* Delete pagetable entry. */ >> if ( unlikely(!UPDATE_ENTRY(l1, pl1e, ol1e, nl1e, gl1mfn, v, 0)) ) >> { >> - MEM_LOG("Cannot delete PTE entry at %p", (unsigned long *)pl1e); >> + gdprintk(XENLOG_WARNING, >> + "Cannot delete PTE entry at %p\n", (unsigned long *)pl1e); > Please drop the stray cast. Oops - thought I had corrected this one. > >> @@ -4316,7 +4367,7 @@ int replace_grant_host_mapping( >> if ( !new_addr ) >> return destroy_grant_pte_mapping(addr, frame, curr->domain); >> >> - MEM_LOG("Unsupported grant table operation"); >> + gdprintk(XENLOG_WARNING, "Unsupported grant table operation\n"); >> return GNTST_general_error; >> } > Drop again? Ok. > >> @@ -4408,10 +4460,11 @@ int donate_page( >> >> fail: >> spin_unlock(&d->page_alloc_lock); >> - MEM_LOG("Bad donate %lx: ed=%d sd=%d caf=%08lx taf=%" PRtype_info, >> - page_to_mfn(page), d->domain_id, >> - owner ? owner->domain_id : DOMID_INVALID, >> - page->count_info, page->u.inuse.type_info); >> + gdprintk(XENLOG_WARNING, "Bad donate mfn %" PRI_mfn >> + ": ed=%d sd=%d caf=%08lx taf=%" PRtype_info "\n", >> + page_to_mfn(page), d->domain_id, >> + owner ? owner->domain_id : DOMID_INVALID, >> + page->count_info, page->u.inuse.type_info); > Why did you not use d%d formatting here? An oversight. > >> @@ -4459,10 +4512,11 @@ int steal_page( >> >> fail: >> spin_unlock(&d->page_alloc_lock); >> - MEM_LOG("Bad page %lx: ed=%d sd=%d caf=%08lx taf=%" PRtype_info, >> - page_to_mfn(page), d->domain_id, >> - owner ? owner->domain_id : DOMID_INVALID, >> - page->count_info, page->u.inuse.type_info); >> + gdprintk(XENLOG_WARNING, "Bad mfn %" PRI_mfn >> + ": ed=%d sd=%d caf=%08lx taf=%" PRtype_info "\n", >> + page_to_mfn(page), d->domain_id, >> + owner ? owner->domain_id : DOMID_INVALID, >> + page->count_info, page->u.inuse.type_info); > Same here. > > Is this intended for 4.9? At this point, yes. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |