Re: [PATCH] x86/mm: Drop MEM_LOG() and correct some printed information

["Jan Beulich" <JBeulich@suse.com>]

>>> On 29.03.17 at 14:29, <andrew.cooper3@citrix.com> 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?

> @@ -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())

> @@ -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.

> @@ -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.

> @@ -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.

> @@ -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?

> @@ -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).

> @@ -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.

> @@ -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.

> @@ -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?

> @@ -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?

> @@ -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?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel