On Wed, 16 May 2018 18:42:01 +0200 Halil Pasic <pa...@linux.ibm.com> wrote:
> On 05/14/2018 06:04 PM, Cornelia Huck wrote: > > On Mon, 14 May 2018 16:22:30 +0200 > > Halil Pasic <pa...@linux.ibm.com> wrote: > > > >> On 05/14/2018 03:45 PM, Cornelia Huck wrote: > >>> On Mon, 14 May 2018 14:40:13 +0200 > >>> Halil Pasic <pa...@linux.ibm.com> wrote: > >>> > >>>> On 05/14/2018 02:18 PM, Cornelia Huck wrote: > >>>>> On Thu, 10 May 2018 02:07:11 +0200 > >>>>> Halil Pasic <pa...@linux.ibm.com> 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. force-orb-pfch looks reasonable to me. > > Can I/do I have to document the property somewhere? Telling the whole > story with the name is going to be difficult. The description member would require that you define your own property type IIUC, which I think would be overkill. So I'd add a comment in the source code. > > >> > >>>> > >>>>>> } 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). A WARN_ONCE (maybe per device) would be the sanest option, I think.