|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3 of 9] x86/mm: Refactor possibly deadlocking get_gfn calls
At 14:51 -0500 on 01 Feb (1328107915), Andres Lagar-Cavilla wrote:
> xen/arch/x86/hvm/emulate.c | 35 +++++++--------
> xen/arch/x86/mm/mem_sharing.c | 28 +++++++------
> xen/include/asm-x86/p2m.h | 91
> +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 122 insertions(+), 32 deletions(-)
>
>
> When calling get_gfn multiple times on different gfn's in the same function,
> we
> can easily deadlock if p2m lookups are locked. Thus, refactor these calls to
> enforce simple deadlock-avoidance rules:
> - Lowest-numbered domain first
> - Lowest-numbered gfn first
This is a good idea, and I like the get_two_gfns() abstraction, but:
- I think the two_gfns struct should proabbly just live on the stack
instead of malloc()ing it up every time. It's not very big.
- the implementation of get_two_gfns() seems to be very complex; I'm
sure it could be simplified. At the very least, you could avoid a
bit of duplication by just deciding once which order to do the gets
in and then running all the setu and get code once.
Cheers,
Tim.
> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxx>
>
> diff -r 27031a8a4eff -r 3de7e43b130a xen/arch/x86/hvm/emulate.c
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -660,12 +660,13 @@ static int hvmemul_rep_movs(
> {
> struct hvm_emulate_ctxt *hvmemul_ctxt =
> container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> - unsigned long saddr, daddr, bytes, sgfn, dgfn;
> + unsigned long saddr, daddr, bytes;
> paddr_t sgpa, dgpa;
> uint32_t pfec = PFEC_page_present;
> - p2m_type_t p2mt;
> + p2m_type_t sp2mt, dp2mt;
> int rc, df = !!(ctxt->regs->eflags & X86_EFLAGS_DF);
> char *buf;
> + struct two_gfns *tg;
>
> rc = hvmemul_virtual_to_linear(
> src_seg, src_offset, bytes_per_rep, reps, hvm_access_read,
> @@ -693,26 +694,25 @@ static int hvmemul_rep_movs(
> if ( rc != X86EMUL_OKAY )
> return rc;
>
> - /* XXX In a fine-grained p2m locking scenario, we need to sort this
> - * get_gfn's, or else we might deadlock */
> - sgfn = sgpa >> PAGE_SHIFT;
> - (void)get_gfn(current->domain, sgfn, &p2mt);
> - if ( !p2m_is_ram(p2mt) && !p2m_is_grant(p2mt) )
> + tg = get_two_gfns(current->domain, sgpa >> PAGE_SHIFT, &sp2mt, NULL,
> NULL,
> + current->domain, dgpa >> PAGE_SHIFT, &dp2mt, NULL,
> NULL,
> + p2m_guest);
> + if ( !tg )
> + return X86EMUL_UNHANDLEABLE;
> +
> + if ( !p2m_is_ram(sp2mt) && !p2m_is_grant(sp2mt) )
> {
> rc = hvmemul_do_mmio(
> sgpa, reps, bytes_per_rep, dgpa, IOREQ_READ, df, NULL);
> - put_gfn(current->domain, sgfn);
> + put_two_gfns(&tg);
> return rc;
> }
>
> - dgfn = dgpa >> PAGE_SHIFT;
> - (void)get_gfn(current->domain, dgfn, &p2mt);
> - if ( !p2m_is_ram(p2mt) && !p2m_is_grant(p2mt) )
> + if ( !p2m_is_ram(dp2mt) && !p2m_is_grant(dp2mt) )
> {
> rc = hvmemul_do_mmio(
> dgpa, reps, bytes_per_rep, sgpa, IOREQ_WRITE, df, NULL);
> - put_gfn(current->domain, sgfn);
> - put_gfn(current->domain, dgfn);
> + put_two_gfns(&tg);
> return rc;
> }
>
> @@ -730,8 +730,7 @@ static int hvmemul_rep_movs(
> */
> if ( ((dgpa + bytes_per_rep) > sgpa) && (dgpa < (sgpa + bytes)) )
> {
> - put_gfn(current->domain, sgfn);
> - put_gfn(current->domain, dgfn);
> + put_two_gfns(&tg);
> return X86EMUL_UNHANDLEABLE;
> }
>
> @@ -743,8 +742,7 @@ static int hvmemul_rep_movs(
> buf = xmalloc_bytes(bytes);
> if ( buf == NULL )
> {
> - put_gfn(current->domain, sgfn);
> - put_gfn(current->domain, dgfn);
> + put_two_gfns(&tg);
> return X86EMUL_UNHANDLEABLE;
> }
>
> @@ -757,8 +755,7 @@ static int hvmemul_rep_movs(
> rc = hvm_copy_to_guest_phys(dgpa, buf, bytes);
>
> xfree(buf);
> - put_gfn(current->domain, sgfn);
> - put_gfn(current->domain, dgfn);
> + put_two_gfns(&tg);
>
> if ( rc == HVMCOPY_gfn_paged_out )
> return X86EMUL_RETRY;
> diff -r 27031a8a4eff -r 3de7e43b130a xen/arch/x86/mm/mem_sharing.c
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -718,11 +718,13 @@ int mem_sharing_share_pages(struct domai
> int ret = -EINVAL;
> mfn_t smfn, cmfn;
> p2m_type_t smfn_type, cmfn_type;
> + struct two_gfns *tg;
>
> - /* XXX if sd == cd handle potential deadlock by ordering
> - * the get_ and put_gfn's */
> - smfn = get_gfn(sd, sgfn, &smfn_type);
> - cmfn = get_gfn(cd, cgfn, &cmfn_type);
> + tg = get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
> + cd, cgfn, &cmfn_type, NULL, &cmfn,
> + p2m_query);
> + if ( !tg )
> + return -ENOMEM;
>
> /* This tricky business is to avoid two callers deadlocking if
> * grabbing pages in opposite client/source order */
> @@ -819,8 +821,7 @@ int mem_sharing_share_pages(struct domai
> ret = 0;
>
> err_out:
> - put_gfn(cd, cgfn);
> - put_gfn(sd, sgfn);
> + put_two_gfns(&tg);
> return ret;
> }
>
> @@ -834,11 +835,13 @@ int mem_sharing_add_to_physmap(struct do
> struct gfn_info *gfn_info;
> struct p2m_domain *p2m = p2m_get_hostp2m(cd);
> p2m_access_t a;
> -
> - /* XXX if sd == cd handle potential deadlock by ordering
> - * the get_ and put_gfn's */
> - smfn = get_gfn_query(sd, sgfn, &smfn_type);
> - cmfn = get_gfn_type_access(p2m, cgfn, &cmfn_type, &a, p2m_query, NULL);
> + struct two_gfns *tg;
> +
> + tg = get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
> + cd, cgfn, &cmfn_type, &a, &cmfn,
> + p2m_query);
> + if ( !tg )
> + return -ENOMEM;
>
> /* Get the source shared page, check and lock */
> ret = XEN_DOMCTL_MEM_SHARING_S_HANDLE_INVALID;
> @@ -886,8 +889,7 @@ int mem_sharing_add_to_physmap(struct do
> err_unlock:
> mem_sharing_page_unlock(spage);
> err_out:
> - put_gfn(cd, cgfn);
> - put_gfn(sd, sgfn);
> + put_two_gfns(&tg);
> return ret;
> }
>
> diff -r 27031a8a4eff -r 3de7e43b130a xen/include/asm-x86/p2m.h
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -378,6 +378,97 @@ static inline unsigned long mfn_to_gfn(s
> return mfn_x(mfn);
> }
>
> +/* Deadlock-avoidance scheme when calling get_gfn on different gfn's */
> +struct two_gfns {
> + struct domain *first_domain;
> + unsigned long first_gfn;
> + struct domain *second_domain;
> + unsigned long second_gfn;
> +};
> +
> +#define assign_pointers(dest, source) \
> +do { \
> + dest ## _mfn = (source ## mfn) ? (source ## mfn) : &__ ## dest ## _mfn;
> \
> + dest ## _a = (source ## a) ? (source ## a) : &__ ## dest ## _a;
> \
> + dest ## _t = (source ## t) ? (source ## t) : &__ ## dest ## _t;
> \
> +} while(0)
> +
> +/* Returns mfn, type and access for potential caller consumption, but any
> + * of those can be NULL */
> +static inline struct two_gfns *get_two_gfns(struct domain *rd, unsigned long
> rgfn,
> + p2m_type_t *rt, p2m_access_t *ra, mfn_t *rmfn, struct domain *ld,
> + unsigned long lgfn, p2m_type_t *lt, p2m_access_t *la, mfn_t *lmfn,
> + p2m_query_t q)
> +{
> + mfn_t *first_mfn, *second_mfn, __first_mfn, __second_mfn;
> + p2m_access_t *first_a, *second_a, __first_a, __second_a;
> + p2m_type_t *first_t, *second_t, __first_t, __second_t;
> +
> + struct two_gfns *rval = xmalloc(struct two_gfns);
> + if ( !rval )
> + return NULL;
> +
> + if ( rd == ld )
> + {
> + rval->first_domain = rval->second_domain = rd;
> +
> + /* Sort by gfn */
> + if (rgfn <= lgfn)
> + {
> + rval->first_gfn = rgfn;
> + rval->second_gfn = lgfn;
> + assign_pointers(first, r);
> + assign_pointers(second, l);
> + } else {
> + rval->first_gfn = lgfn;
> + rval->second_gfn = rgfn;
> + assign_pointers(first, l);
> + assign_pointers(second, r);
> + }
> + } else {
> + /* Sort by domain */
> + if ( rd->domain_id <= ld->domain_id )
> + {
> + rval->first_domain = rd;
> + rval->first_gfn = rgfn;
> + rval->second_domain = ld;
> + rval->second_gfn = lgfn;
> + assign_pointers(first, r);
> + assign_pointers(second, l);
> + } else {
> + rval->first_domain = ld;
> + rval->first_gfn = lgfn;
> + rval->second_domain = rd;
> + rval->second_gfn = rgfn;
> + assign_pointers(first, l);
> + assign_pointers(second, r);
> + }
> + }
> +
> + /* Now do the gets */
> + *first_mfn = get_gfn_type_access(p2m_get_hostp2m(rval->first_domain),
> + rval->first_gfn, first_t, first_a, q,
> NULL);
> + *second_mfn = get_gfn_type_access(p2m_get_hostp2m(rval->second_domain),
> + rval->second_gfn, second_t, second_a,
> q, NULL);
> +
> + return rval;
> +}
> +
> +static inline void put_two_gfns(struct two_gfns **arg_ptr)
> +{
> + struct two_gfns *arg;
> +
> + if ( !arg_ptr || !(*arg_ptr) )
> + return;
> +
> + arg = *arg_ptr;
> + put_gfn(arg->second_domain, arg->second_gfn);
> + put_gfn(arg->first_domain, arg->first_gfn);
> +
> + xfree(arg);
> + *arg_ptr = NULL;
> +}
> +
> /* Init the datastructures for later use by the p2m code */
> int p2m_init(struct domain *d);
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |