Re: [Xen-devel] [PATCH v3 08/15] argo: implement the unregister op

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Mon, Jan 14, 2019 at 7:06 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 07.01.19 at 08:42, <christopher.w.clark@xxxxxxxxx> wrote:
> > @@ -666,6 +667,105 @@ ring_find_info(const struct domain *d, const struct 
> > argo_ring_id *id)
> >      return NULL;
> >  }
> >
> > +static struct argo_send_info *
> > +send_find_info(const struct domain *d, const struct argo_ring_id *id)
>
> As per the comment on patch 7, perhaps find_send_info()?

ack, yes, renamed.

>
> > +{
> > +    struct hlist_node *node;
>
> const?

understood; not got this one done yet, but yes.

>
> > +static long
> > +unregister_ring(struct domain *currd,
> > +                XEN_GUEST_HANDLE_PARAM(xen_argo_unregister_ring_t) 
> > unreg_hnd)
> > +{
> > +    xen_argo_unregister_ring_t unreg;
> > +    struct argo_ring_id ring_id;
> > +    struct argo_ring_info *ring_info;
> > +    struct argo_send_info *send_info;
> > +    struct domain *dst_d = NULL;
> > +    int ret;
> > +
> > +    ret = copy_from_guest(&unreg, unreg_hnd, 1) ? -EFAULT : 0;
> > +    if ( ret )
> > +        goto out;
> > +
> > +    ret = unreg.pad ? -EINVAL : 0;
> > +    if ( ret )
> > +        goto out;
> > +
> > +    ring_id.partner_id = unreg.partner_id;
> > +    ring_id.port = unreg.port;
> > +    ring_id.domain_id = currd->domain_id;
> > +
> > +    read_lock(&argo_lock);
> > +
> > +    if ( !currd->argo )
> > +    {
> > +        ret = -ENODEV;
> > +        goto out_unlock;
> > +    }
> > +
> > +    write_lock(&currd->argo->lock);
> > +
> > +    ring_info = ring_find_info(currd, &ring_id);
> > +    if ( ring_info )
> > +    {
> > +        ring_remove_info(currd, ring_info);
> > +        currd->argo->ring_count--;
> > +    }
> > +
> > +    dst_d = get_domain_by_id(ring_id.partner_id);
> > +    if ( dst_d )
> > +    {
> > +        if ( dst_d->argo )
> > +        {
> > +            spin_lock(&dst_d->argo->send_lock);
> > +
> > +            send_info = send_find_info(dst_d, &ring_id);
> > +            if ( send_info )
> > +            {
> > +                hlist_del(&send_info->node);
> > +                xfree(send_info);
> > +            }
> > +
> > +            spin_unlock(&dst_d->argo->send_lock);
>
> As per the comment to an earlier patch, if at all possible call
> allocation (and hence also freeing) functions with as little
> locks held as possible. Pulling it out of the innermost lock
> here looks straightforward at least.

ack, have pulled it out of both L2s now which will help.

thanks

Christopher

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