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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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