|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] Fix rcu domain locking for transitive grants
On 08/03/2011 15:42, "George Dunlap" <george.dunlap@xxxxxxxxxx> wrote:
> On Tue, 2011-03-08 at 15:38 +0000, Keir Fraser wrote:
>> On 08/03/2011 15:11, "George Dunlap" <George.Dunlap@xxxxxxxxxxxxx> wrote:
>>
>>> This should be backported to the 4.1 branch; it causes a hypervisor
>>> BUG() if guests are using netchannel2 transtiive grants to talk to
>>> each other when debug mode is on.
>>
>> I stubbed out the preemption checking stuff in 4.1 branch (it's not really
>> needed since there are no users of waitqueues in 4.1), so this patch is not
>> required. And that's fortunate, since it's quite non-trivial.
>
> Are the rcu locks still noops then?
Yes, since preempt_disable/enable are noops.
-- Keir
> -George
>
>
>>
>> -- Keir
>>
>>> -George
>>>
>>> On Tue, Mar 8, 2011 at 3:02 PM, George Dunlap
>>> <george.dunlap@xxxxxxxxxxxxx> wrote:
>>>> When acquiring a transitive grant for copy then the owning domain needs to
>>>> be locked down as well as the granting domain. This was being done, but the
>>>> unlocking was not. The acquire code now stores the struct domain * of the
>>>> owning domain (rather than the domid) in the active entry in the granting
>>>> domain. The release code then does the unlock on the owning domain.
>>>> Note that I believe I also fixed a bug where, for non-transitive grants
>>>> the active entry contained a reference to the acquiring domain rather
>>>> than the granting domain. From my reading of the code this would stop the
>>>> release code for transitive grants from terminating its recursion
>>>> correctly.
>>>>
>>>> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
>>>> CC: Steven Smith <steven.smith@xxxxxxxxxx>
>>>>
>>>> diff -r f071d8e9f744 -r 14211e98efac xen/common/grant_table.c
>>>> --- a/xen/common/grant_table.c Tue Mar 08 10:23:52 2011 +0000
>>>> +++ b/xen/common/grant_table.c Tue Mar 08 14:39:03 2011 +0000
>>>> @@ -1626,11 +1626,10 @@
>>>> struct active_grant_entry *act;
>>>> unsigned long r_frame;
>>>> uint16_t *status;
>>>> - domid_t trans_domid;
>>>> grant_ref_t trans_gref;
>>>> int released_read;
>>>> int released_write;
>>>> - struct domain *trans_dom;
>>>> + struct domain *td;
>>>>
>>>> released_read = 0;
>>>> released_write = 0;
>>>> @@ -1644,15 +1643,13 @@
>>>> if (rd->grant_table->gt_version == 1)
>>>> {
>>>> status = &sha->flags;
>>>> - trans_domid = rd->domain_id;
>>>> - /* Shut the compiler up. This'll never be used, because
>>>> - trans_domid == rd->domain_id, but gcc doesn't know that. */
>>>> - trans_gref = 0x1234567;
>>>> + td = rd;
>>>> + trans_gref = gref;
>>>> }
>>>> else
>>>> {
>>>> status = &status_entry(rd->grant_table, gref);
>>>> - trans_domid = act->trans_dom;
>>>> + td = act->trans_domain;
>>>> trans_gref = act->trans_gref;
>>>> }
>>>>
>>>> @@ -1680,21 +1677,16 @@
>>>>
>>>> spin_unlock(&rd->grant_table->lock);
>>>>
>>>> - if ( trans_domid != rd->domain_id )
>>>> + if ( td != rd )
>>>> {
>>>> - if ( released_write || released_read )
>>>> - {
>>>> - trans_dom = rcu_lock_domain_by_id(trans_domid);
>>>> - if ( trans_dom != NULL )
>>>> - {
>>>> - /* Recursive calls, but they're tail calls, so it's
>>>> - okay. */
>>>> - if ( released_write )
>>>> - __release_grant_for_copy(trans_dom, trans_gref, 0);
>>>> - else if ( released_read )
>>>> - __release_grant_for_copy(trans_dom, trans_gref, 1);
>>>> - }
>>>> - }
>>>> + /* Recursive calls, but they're tail calls, so it's
>>>> + okay. */
>>>> + if ( released_write )
>>>> + __release_grant_for_copy(td, trans_gref, 0);
>>>> + else if ( released_read )
>>>> + __release_grant_for_copy(td, trans_gref, 1);
>>>> +
>>>> + rcu_unlock_domain(td);
>>>> }
>>>> }
>>>>
>>>> @@ -1731,7 +1723,7 @@
>>>> uint32_t old_pin;
>>>> domid_t trans_domid;
>>>> grant_ref_t trans_gref;
>>>> - struct domain *rrd;
>>>> + struct domain *td;
>>>> unsigned long gfn;
>>>> unsigned long grant_frame;
>>>> unsigned trans_page_off;
>>>> @@ -1785,8 +1777,8 @@
>>>> status) ) != GNTST_okay )
>>>> goto unlock_out;
>>>>
>>>> - trans_domid = ld->domain_id;
>>>> - trans_gref = 0;
>>>> + td = rd;
>>>> + trans_gref = gref;
>>>> if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
>>>> {
>>>> if ( !allow_transitive )
>>>> @@ -1808,14 +1800,15 @@
>>>> that you don't need to go out of your way to avoid it
>>>> in the guest. */
>>>>
>>>> - rrd = rcu_lock_domain_by_id(trans_domid);
>>>> - if ( rrd == NULL )
>>>> + /* We need to leave the rrd locked during the grant copy */
>>>> + td = rcu_lock_domain_by_id(trans_domid);
>>>> + if ( td == NULL )
>>>> PIN_FAIL(unlock_out, GNTST_general_error,
>>>> "transitive grant referenced bad domain %d\n",
>>>> trans_domid);
>>>> spin_unlock(&rd->grant_table->lock);
>>>>
>>>> - rc = __acquire_grant_for_copy(rrd, trans_gref, rd,
>>>> + rc = __acquire_grant_for_copy(td, trans_gref, rd,
>>>> readonly, &grant_frame,
>>>> &trans_page_off, &trans_length,
>>>> 0, &ignore);
>>>> @@ -1823,6 +1816,7 @@
>>>> spin_lock(&rd->grant_table->lock);
>>>> if ( rc != GNTST_okay ) {
>>>> __fixup_status_for_pin(act, status);
>>>> + rcu_unlock_domain(td);
>>>> spin_unlock(&rd->grant_table->lock);
>>>> return rc;
>>>> }
>>>> @@ -1834,6 +1828,7 @@
>>>> if ( act->pin != old_pin )
>>>> {
>>>> __fixup_status_for_pin(act, status);
>>>> + rcu_unlock_domain(td);
>>>> spin_unlock(&rd->grant_table->lock);
>>>> return __acquire_grant_for_copy(rd, gref, ld, readonly,
>>>> frame, page_off, length,
>>>> @@ -1845,7 +1840,7 @@
>>>> sub-page, but we always treat it as one because that
>>>> blocks mappings of transitive grants. */
>>>> is_sub_page = 1;
>>>> - *owning_domain = rrd;
>>>> + *owning_domain = td;
>>>> act->gfn = -1ul;
>>>> }
>>>> else if ( sha1 )
>>>> @@ -1891,7 +1886,7 @@
>>>> act->is_sub_page = is_sub_page;
>>>> act->start = trans_page_off;
>>>> act->length = trans_length;
>>>> - act->trans_dom = trans_domid;
>>>> + act->trans_domain = td;
>>>> act->trans_gref = trans_gref;
>>>> act->frame = grant_frame;
>>>> }
>>>> diff -r f071d8e9f744 -r 14211e98efac xen/include/xen/grant_table.h
>>>> --- a/xen/include/xen/grant_table.h Tue Mar 08 10:23:52 2011 +0000
>>>> +++ b/xen/include/xen/grant_table.h Tue Mar 08 14:39:03 2011 +0000
>>>> @@ -32,7 +32,7 @@
>>>> struct active_grant_entry {
>>>> u32 pin; /* Reference count information. */
>>>> domid_t domid; /* Domain being granted access. */
>>>> - domid_t trans_dom;
>>>> + struct domain *trans_domain;
>>>> uint32_t trans_gref;
>>>> unsigned long frame; /* Frame being granted. */
>>>> unsigned long gfn; /* Guest's idea of the frame being granted. */
>>>>
>>>> _______________________________________________
>>>> 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
>>
>>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |