Re: [Xen-devel] [PATCH v5 11/18] x86/mem_sharing: Replace MEM_SHARING_DEBUG with gdprintk

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Jan 22, 2020 at 8:30 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 21.01.2020 18:49, Tamas K Lengyel wrote:
> > @@ -538,24 +535,26 @@ static int audit(void)
> >              d = get_domain_by_id(g->domain);
> >              if ( d == NULL )
> >              {
> > -                MEM_SHARING_DEBUG("Unknown dom: %hu, for PFN=%lx, 
> > MFN=%lx\n",
> > -                                  g->domain, g->gfn, mfn_x(mfn));
> > +                gdprintk(XENLOG_ERR,
> > +                         "Unknown dom: %pd, for PFN=%lx, MFN=%lx\n",
> > +                         d, g->gfn, mfn_x(mfn));
>
> With "if ( d == NULL )" around this you hardly mean to pass d to
> the function here. This is a case where you really need to stick
> to logging a raw number.

Indeed..

>
> >                  errors++;
> >                  continue;
> >              }
> >              o_mfn = get_gfn_query_unlocked(d, g->gfn, &t);
> >              if ( !mfn_eq(o_mfn, mfn) )
> >              {
> > -                MEM_SHARING_DEBUG("Incorrect P2M for d=%hu, PFN=%lx."
> > -                                  "Expecting MFN=%lx, got %lx\n",
> > -                                  g->domain, g->gfn, mfn_x(mfn), 
> > mfn_x(o_mfn));
> > +                gdprintk(XENLOG_ERR, "Incorrect P2M for d=%pd, PFN=%lx."
>
> Here and elsewhere may I recommend dropping d= (or dom= further
> down)?

SGTM

>
> > @@ -757,10 +756,10 @@ static int debug_mfn(mfn_t mfn)
> >          return -EINVAL;
> >      }
> >
> > -    MEM_SHARING_DEBUG(
> > -        "Debug page: MFN=%lx is ci=%lx, ti=%lx, owner=%pd\n",
> > -        mfn_x(page_to_mfn(page)), page->count_info,
> > -        page->u.inuse.type_info, page_get_owner(page));
> > +    gdprintk(XENLOG_ERR,
> > +             "Debug page: MFN=%lx is ci=%lx, ti=%lx, owner_id=%d\n",
> > +             mfn_x(page_to_mfn(page)), page->count_info,
> > +             page->u.inuse.type_info, page_get_owner(page)->domain_id);
>
> As indicated before (I think), please prefer %pd and a struct domain
> pointer over passing ->domain_id (at least one more instance further
> down).

I thought I fixed them all but evidently some remained.

Thanks,
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel