On Tue, 13 Aug 2019 10:55:21 +0200, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> On 09/08/19 17:59, Adalbert Lazăr wrote:
> >
> > + reply->padding2);
> > +
> > + ivcpu->reply_waiting = false;
> > + return expected->error;
> > +}
> > +
> > /*
>
> Is this missing a wakeup?
>
> >
> > +static bool need_to_wait(struct kvm_vcpu *vcpu)
> > +{
> > + struct kvmi_vcpu *ivcpu = IVCPU(vcpu);
> > +
> > + return ivcpu->reply_waiting;
> > +}
> > +
>
> Do you actually need this function? It seems to me that everywhere you
> call it you already have an ivcpu, so you can just access the field.
>
> Also, "reply_waiting" means "there is a reply that is waiting". What
> you mean is "waiting_for_reply".
In an older version, handle_event_reply() was executed from the receiving
thread (having another name) and it contained a wakeup function. Now,
indeed, 'waiting_for_reply' is the right name.
> The overall structure of the jobs code is confusing. The same function
> kvm_run_jobs_and_wait is an infinite loop before and gets a "break"
> later. It is also not clear why kvmi_job_wait is called through a job.
> Can you have instead just kvm_run_jobs in KVM_REQ_INTROSPECTION, and
> something like this instead when sending an event:
>
> int kvmi_wait_for_reply(struct kvm_vcpu *vcpu)
> {
> struct kvmi_vcpu *ivcpu = IVCPU(vcpu);
>
> while (ivcpu->waiting_for_reply) {
> kvmi_run_jobs(vcpu);
>
> err = swait_event_killable(*wq,
> !ivcpu->waiting_for_reply ||
> !list_empty(&ivcpu->job_list));
>
> if (err)
> return -EINTR;
> }
>
> return 0;
> }
>
> ?
>
> Paolo
Much better :) Thank you.
