Re: [Xen-devel] [PATCH v2 01/11] vpci: move lock

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.07.18 at 11:48, <roger.pau@xxxxxxxxxx> wrote:
> @@ -62,12 +70,15 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
>      if ( !has_vpci(pdev->domain) )
>          return 0;
>  
> +    spin_lock(&pdev->vpci_lock);
>      pdev->vpci = xzalloc(struct vpci);

Patterns like this should be avoided where possible: I'd prefer if you
called xzalloc() before acquiring the lock, storing the result in a local
variable.

Also the locked region becomes pretty big this way.

> @@ -148,8 +160,6 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t 
> *read_handler,
>      r->offset = offset;
>      r->private = data;
>  
> -    spin_lock(&vpci->lock);
> -
>      /* The list of handlers must be kept sorted at all times. */
>      list_for_each ( prev, &vpci->handlers )
>      {
> @@ -161,14 +171,12 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t 
> *read_handler,
>              break;
>          if ( cmp == 0 )
>          {
> -            spin_unlock(&vpci->lock);
>              xfree(r);
>              return -EEXIST;
>          }
>      }
>  
>      list_add_tail(&r->node, prev);
> -    spin_unlock(&vpci->lock);
>  
>      return 0;
>  }

With the above in mind, this isn't very nice, because it puts an xmalloc()
invocation inside a locked region.

Can't you solve the race you mention in the description by simply latching
pdev->vpci into a local variable in vpci_remove_device(), and storing
NULL into the field with the lock still held? That way all the xfree()s
there could apparently move out of the locked region (if need be by
deferring all of this to an RCU callback).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel