kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Halil Pasic <pasic@linux.ibm.com>
To: Eric Farman <farman@linux.ibm.com>
Cc: Cornelia Huck <cohuck@redhat.com>,
	Matthew Rosato <mjrosato@linux.ibm.com>,
	Jared Rossi <jrossi@linux.ibm.com>,
	linux-s390@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH v6 0/3] vfio-ccw: Fix interrupt handling for HALT/CLEAR
Date: Thu, 13 May 2021 03:05:43 +0200	[thread overview]
Message-ID: <20210513030543.67601a8c.pasic@linux.ibm.com> (raw)
In-Reply-To: <20210511195631.3995081-1-farman@linux.ibm.com>

On Tue, 11 May 2021 21:56:28 +0200
Eric Farman <farman@linux.ibm.com> wrote:

> Hi Conny, Matt, Halil,
> 
> Here's one (last?) update to my proposal for handling the collision
> between interrupts for START SUBCHANNEL and HALT/CLEAR SUBCHANNEL.
> 
> Only change here is to include Conny's suggestions on patch 3.
> 
> Thanks,

I believe these changes are beneficial, although I don't understand
everything about them. In that sense I'm happy with the these getting
merged.

Let me also spend some words answering the unasked question, what I'm
not understanding about these.

Not understanding how the problem stated in the cover letter of v4 is
actually resolved is certainly the most important one. Let me cite
the relevant part of it (your cover letter already contains a link to
the full version).

"""

	CPU 1			CPU 2
 1	CLEAR SUBCHANNEL
 2	fsm_irq()
 3				START SUBCHANNEL
 4	vfio_ccw_sch_io_todo()
 5				fsm_irq()
 6				vfio_ccw_sch_io_todo()

From the channel subsystem's point of view the CLEAR SUBCHANNEL (step 1)
is complete once step 2 is called, as the Interrupt Response Block (IRB)
has been presented and the TEST SUBCHANNEL was driven by the cio layer.
Thus, the START SUBCHANNEL (step 3) is submitted [1] and gets a cc=0 to
indicate the I/O was accepted. However, step 2 stacks the bulk of the
actual work onto a workqueue for when the subchannel lock is NOT held,
and is unqueued at step 4. That code misidentifies the data in the IRB
as being associated with the newly active I/O, and may release memory
that is actively in use by the channel subsystem and/or device. Eww.
"""

The last sentence clearly states "may release memory that is actively
used by ... the device", and I understood it refers to the invocation
of cp_free() from vfio_ccw_sch_io_todo(). Patch 3 of this series does
not change the conditions under which cp_free() is called.

Looking at the cited diagram, since patch 3 changes things in
vfio_ccw_sch_io_todo() it probably ain't affecting steps 1-3 and
I understood the description so that bad free happens in step 4.

My guess is that your change from patch 3 somehow via the fsm prevents
the SSCH on CPU 2 (using the diagram) from being executed  if it actually
happens to be after vfio_ccw_sch_io_todo(). And patch 1 is supposed to
prevent the SSCH on CPU2 from being executed in the depicted case because
if there is a cp to free, then we would bail out form if we see it
while processing the new IO request.

In any case, I don't want to hold this up any further.

Regards,
Halil

  parent reply	other threads:[~2021-05-13  1:05 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-11 19:56 Eric Farman
2021-05-11 19:56 ` [PATCH v6 1/3] vfio-ccw: Check initialized flag in cp_init() Eric Farman
2021-05-11 20:10   ` Matthew Rosato
2021-05-11 19:56 ` [PATCH v6 2/3] vfio-ccw: Reset FSM state to IDLE inside FSM Eric Farman
2021-05-11 20:38   ` Matthew Rosato
2021-05-11 19:56 ` [PATCH v6 3/3] vfio-ccw: Serialize FSM IDLE state with I/O completion Eric Farman
2021-05-11 20:45   ` Matthew Rosato
2021-05-12 10:49   ` Cornelia Huck
2021-05-13  1:05 ` Halil Pasic [this message]
2021-05-13 18:33   ` [PATCH v6 0/3] vfio-ccw: Fix interrupt handling for HALT/CLEAR Eric Farman
2021-05-14  0:29     ` Halil Pasic
2021-05-18  9:49 ` Cornelia Huck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210513030543.67601a8c.pasic@linux.ibm.com \
    --to=pasic@linux.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=farman@linux.ibm.com \
    --cc=jrossi@linux.ibm.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=mjrosato@linux.ibm.com \
    --subject='Re: [PATCH v6 0/3] vfio-ccw: Fix interrupt handling for HALT/CLEAR' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).