[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] xen/xsm: Improve alloc/free of evtchn buckets

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Mon, 18 Jan 2021 16:21:20 +0000
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=MUE5KSfQ1D9QkJttZtVwfLYaRSpznM56rnpe/sFqHkI=; b=NM+yyEtCBV1G1EAH10FyHKU3HPlAQEAjwbAxhhlOPGEGFSOdkjau8INaUAE3EZuZW22WcyNp0pwnnN4hrNHtXclGOZoNe15EEztFX/2Fs4D0bfpkpzbi4W9icm6xtSIwDQK4vZMiHn5ys7t8p4E7zev+SCiAuPfmwLeK8eTmEK8zeMz3NVjitBNZfhRHtEi1FQfqCfjJsmlDRPGR/90O9YDt1gTUn1IOhBL3b0tXbHUyCx+tYxWN+iq8troO0JhLEICOy6PMN9JuwcXrrXVFdqxHdlmMwVYuvy/GbxKCGm+1egCJynbXhINhnGC/w1+z0rftDVK9Qs4149PnFHN8Lw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=aqdZ/tFtc6v6wy2rvfC9wWFUeytS8MgOeiN60U5E2VqLSEnG4FvEOqqRN6aTm7y8ak8WJIexwDHCCh1T2BiVgFqZ1yicB6kL6bhU6JRiU6YXP32g6DhpreV9QrGoKSpl0crU/T0OG2r5aJmx8yTfm6iunoR4r1HYeyS86r8jcYuRK78Was5t8Q7hCGynzu7r2bDISuWNLs5PRbC+j2IXX0ifBk9Xd9qT5Qq1hF/tt6UwsZGgPVIHsGcqSdqGVx+w8wFnCZ0wr1oOPkvAOrmnn6cQJrL9K/SUbjGddxcgG1UWif4yXfedJpsdWoMSpruohaBHEIFmpJ12UP8M8vmMIA==
  • Authentication-results: esa6.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>, "Daniel P . Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 18 Jan 2021 16:21:35 +0000
  • Ironport-sdr: 9H0UqI1NVOtx9dWOeGjOFJHweE9HpCr8J4LPDqBCk+spLkw3438hpCO6rvw9BKQGnrZX2VBsjT zLQOJNo9Stp9kDKDovq6K7ph/ZkqOtvaV8fBDIOYyFUhXeo9zoOJ0vJKyeLPhkz8osloTQZUpz u+PN/jhE4iKfq0QKN0/6NbFw03jiG9uD5il/ZGsPqtSUoRQgY8fltERB/VOkNVo+pJJRXSvNLu L0URzE69KtfeX8kAF8K7847G2cyFnGEtNdpqUSgdcGnKtVp3mhNv/IBN+w2b7Uc8NNQ5smanWW iNM=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 18/01/2021 15:31, Jan Beulich wrote:
> On 18.01.2021 16:06, Andrew Cooper wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -147,6 +147,14 @@ static bool virq_is_global(unsigned int virq)
>>      return true;
>>  }
>> +static void free_evtchn_bucket(struct domain *d, struct evtchn *bucket)
>> +{
>> +    if ( !bucket )
>> +        return;
> You could avoid this since flask_free_security_evtchns() has
> a similar check. Alternatively it could be dropped from there.

I considered altering both.  However, all functions like this really
should be idempotent.

For this case, the compiler can drop the check from both callsites, and
its safer if the structure of the callers change in the future.

> But even if you want to keep the duplication
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>


> One further aspect to consider though:
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -309,12 +309,12 @@ static XSM_INLINE int xsm_evtchn_reset(XSM_DEFAULT_ARG 
>> struct domain *d1, struct
>>      return xsm_default_action(action, d1, d2);
>>  }
>> -static XSM_INLINE int xsm_alloc_security_evtchn(struct evtchn *chn)
>> +static XSM_INLINE int xsm_alloc_security_evtchns(struct evtchn *chn, 
>> unsigned int nr)
> I wonder whether we wouldn't better identify the difference
> between pointer (to individual element) and array by writing
> this (and others below) as
> static XSM_INLINE int xsm_alloc_security_evtchns(struct evtchn chn[], 
> unsigned int nr)

In which case want we want is (unsigned int nr, struct evtchn chn[nr])
which I think is the C99 way of writing this to help static analysis.

> I think we've done so in a few places already, but of course it
> would be a long way to get the entire code base consistent in
> this regard. Plus of course while this works fine in function
> declarations / definitions, it won't be possible to use for
> struct / union fields.
> Also it looks like this and further lines have become overly long.

Everything is long lines in this area of code.  Its all due an overhaul.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.