Re: [Qemu-devel] [qemu-s390x] [PATCH 1/2] vfio-ccw: add force unlimited prefetch property

[Halil Pasic]

On 05/14/2018 06:04 PM, Cornelia Huck wrote:
> On Mon, 14 May 2018 16:22:30 +0200
> Halil Pasic <address@hidden> wrote:
>
> > On 05/14/2018 03:45 PM, Cornelia Huck wrote:
> > > On Mon, 14 May 2018 14:40:13 +0200
> > > Halil Pasic <address@hidden> wrote:
> > >    
> > > > On 05/14/2018 02:18 PM, Cornelia Huck wrote:
> > > > > On Thu, 10 May 2018 02:07:11 +0200
> > > > > Halil Pasic <address@hidden> wrote:

[..]

> > > > > > +    bool f_upfch;
> > > > >
> > > > > force_unlimited_prefetch? You only use it that often :)
> > > > >       
> > > >
> > > > I would have expected complaints for the property name in the
> > > > first place. I think we should first find a good name for the
> > > > property and then consider the rest.
> > >
> > > What about 'force_pfch' (at least matches the name of the bit in the
> > > code)?
> > >    
> >
> > I like upfch more as it really not about forcing any prefetch, but
> > allowing *unlimited* prefetch for the channel program.
>
> 'always_allow_prefetch', then? The problem is that we force a flag to
> be set, which does not force but allow something. Hard to express in a
> short property name :(
>
> Any other suggestions?

How about force-orb-pfch or force-orb-pbit (PoP name) for the property
and with underscores for the variable. My idea was (starting from your
force_pfch) to spell out that we are fiddling with that orb bit.

Can I/do I have to document the property somewhere? Telling the whole
story with the name is going to be difficult.

> > > >   
> > > > > >     } VFIOCCWDevice;
>
> > > > > > @@ -429,6 +441,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error 
> > > > > > **errp)
> > > > > >     
> > > > > >     static Property vfio_ccw_properties[] = {
> > > > > >         DEFINE_PROP_STRING("sysfsdev", VFIOCCWDevice, vdev.sysfsdev),
> > > > > > +    DEFINE_PROP_BOOL("f-upfch", VFIOCCWDevice, f_upfch, false),

[..]

> > > Another thought: Should there be a warning logged somewhere if we
> > > actually force pfch (i.e., not just set the property)?
> > >    
> >
> > I don't think so. With libvirt the cmd line gets logged. So we can
> > tell if the machine was running with this forced or not. This knob
> > is really (an opt-in) for expert users only.
>
> But there's a difference between 'we set this one preemptively' and 'we
> set it, and the guest actually did a request with pfch off'.

I expect the admin to set it *only* after seeing SSCHs fail with
the 'vfio-ccw requires PFCH flag set' message. So no preemptive usage
is expected, but...

> > Furthermore a warning about this may not be very constructive,
> > as there is not much that can be done to make the warning go away.
> > IMHO getting used to warnings is not a good thing.
> >
> > Or am I missing a reason for issuing a warning?
>
> Just log this once so that the admin sees 'yes, the guest actually did
> a request with pfch off, so if funny things happened, that might be the
> reason'. Of course, if this is only an edge use case, that would be
> overkill.

... if the admin (actually more likely developer/tester) is mistaken
about the guest, and it does require the guarantees, but things don't blow
up right away, this message, together with the timestamp could help
determine why things turned funny.

So I do see the benefit now. But then I wonder if it should be a
WARN_ONCE type thing, or if we shall issue a warning each time the override
happens. (Considering the laid out expected benefit, if the first request
is OK but some subsequent one is not OK (needs the conservative prefetch
behavior) we don't gain much).

Regards,
Halil