Messages in this thread | | | From | Dionna Amalie Glaze <> | Date | Fri, 28 Oct 2022 08:57:51 -0700 | Subject | Re: [PATCH v4 4/4] virt/coco/sev-guest: interpret VMM errors from guest request |
| |
> > static u32 vmpck_id; > > module_param(vmpck_id, uint, 0444); > > MODULE_PARM_DESC(vmpck_id, "The VMPCK ID to use when communicating with the PSP."); > > > > +static int rate_hz = 2; > > s/int/unsigned int/ to match uint below. >
I'll change the uint to int, since the types in ratelimit_struct are int.
> > +module_param(rate_burst, uint, 0444); > > +MODULE_PARM_DESC(rate_burst, "The rate limit burst amount to limit requests to."); > > + > > /* Mutex to serialize the shared buffer access and command handling. */ > > static DEFINE_MUTEX(snp_cmd_mutex); > > > > @@ -305,9 +316,12 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in > > u8 type, void *req_buf, size_t req_sz, void *resp_buf, > > u32 resp_sz, __u64 *exitinfo2) > > { > > + unsigned int vmm_err; > > u64 seqno; > > int rc; > > > > + might_resched(); > > + > > Not sure this is needed. This may call cond_resched() right away, so I > don't think this is doing what you think it is (as I only see it used in > scheduler code and spinlock code). Did you mean to use might_sleep()?
Rereading the docs about it, I'll remove it.
> > > /* Get message sequence and verify that its a non-zero */ > > seqno = snp_get_msg_seqno(snp_dev); > > if (!seqno) > > @@ -320,9 +334,35 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in > > if (rc) > > return rc; > > > > +retry: > > + /* > > + * Rate limit commands internally since the host can also throttle, and > > + * we don't want to create a tight request spin that could end up > > + * getting this VM throttled more heavily. > > + */ > > + if (!__ratelimit(&snp_dev->rs)) { > > This doesn't do any sleeping/delaying, it just returns a 0 or 1, so you > could still cause a tight spin here. I guess that shouldn't be a problem > is nothing else is ready to run. But maybe adding an msleep()/usleep() > here based on the rate limit parameters (half the rate limit?) would be > good so that the CPU isn't pegged while rating for the ratelimit to be > satisified? >
It looks like sleep_timeout_interruptible((rate_hz * HZ) / 2) could be the appropriate solution for this.
> I'll let others chime in on that and see if it is appropriate. >
Same, I'll wait until later Monday to send out v5.
> Thanks, > Tom
Thanks for the reviews :)
-- -Dionna Glaze, PhD (she/her)
| |