Great! looking forward to the test results.
On Fri, Feb 13, 2015 at 3:56 AM, Salvatore Cambria
<salvatore.camb...@citrix.com> wrote:
> Hi Andy,
>
> Yes, it makes a lot of sense!
>
> I applied the patch and I'm running my script again in order to test it. It
> looks good so far!
> I will leave the script running all across the weekend and I'll come back to
> you with my results on Monday.
>
> Thank you for your very fast response and for your effort,
>
> Salvatore
>
> On 13/02/15 02:10, Andy Zhou wrote:
>>
>> Does not mean to drop the list.
>>
>>
>> ---------- Forwarded message ----------
>> From: Andy Zhou <az...@nicira.com>
>> Date: Thu, Feb 12, 2015 at 3:42 PM
>> Subject: Re: [ovs-discuss] OVS segfault in recirculation
>> To: Salvatore Cambria <salvatore.camb...@citrix.com>
>>
>>
>> Hi, Salvatore,
>>
>> I think I found a bug: hmap_remove needs to be protected by a lock. As
>> you pointed out, the output_normal() path is not properly protected.
>> Would you please
>> try the attached patch to see if it helps?
>>
>> Bond object is protected by reference counting. Assuming reference
>> counter is valid, it should be O.K. for a bundle to hold on to the
>> object. Notice
>> bundle_destroy() calls unref_bond(). At any rate, I did not spot
>> anything obvious there.
>>
>> Thanks for providing such a detailed description. It is very helpful.
>>
>> Andy
>>
>> ================================
>>
>> iff --git a/ofproto/bond.c b/ofproto/bond.c
>> index c4b3a3a..a9b7a83 100644
>> --- a/ofproto/bond.c
>> +++ b/ofproto/bond.c
>> @@ -923,8 +923,8 @@ bond_may_recirc(const struct bond *bond, uint32_t
>> *recirc_id,
>> }
>> }
>>
>> -void
>> -bond_update_post_recirc_rules(struct bond* bond, const bool force)
>> +static void
>> +bond_update_post_recirc_rules__(struct bond* bond, const bool force)
>> {
>> struct bond_entry *e;
>> bool update_rules = force; /* Always update rules if caller forces
>> it. */
>> @@ -945,6 +945,14 @@ bond_update_post_recirc_rules(struct bond* bond,
>> const bool force)
>> update_recirc_rules(bond);
>> }
>> }
>> +
>> +void
>> +bond_update_post_recirc_rules(struct bond* bond, const bool force)
>> +{
>> + ovs_rwlock_wrlock(&rwlock);
>> + bond_update_post_recirc_rules__(bond, force);
>> + ovs_rwlock_unlock(&rwlock);
>> +}
>> ^L
>> /* Rebalancing. */
>>
>> @@ -1203,7 +1211,7 @@ bond_rebalance(struct bond *bond)
>> }
>>
>> if (use_recirc && rebalanced) {
>> - bond_update_post_recirc_rules(bond,true);
>> + bond_update_post_recirc_rules__(bond,true);
>> }
>>
>> done:
>>
>>
>> On Tue, Feb 10, 2015 at 7:57 AM, Salvatore Cambria
>> <salvatore.camb...@citrix.com> wrote:
>>>
>>> Hello,
>>>
>>> I am a Software Engineer from Citrix, Cambridge UK office.
>>>
>>> We are having a problem when running our nightly test on a XenServer host
>>> with lacp bonds with OVS. Indeed, when deleting the bond the following
>>> segfault error appears (from GDB):
>>>
>>> Program terminated with signal 11, Segmentation fault.
>>>
>>> #0 0x0000000000411d2f in hmap_remove (node=0x7fd9d402e730,
>>> hmap=0x1c3e9e8)
>>>
>>> at lib/hmap.h:236
>>>
>>> 236 while (*bucket != node) {
>>>
>>> (gdb) bt
>>>
>>> #0 0x0000000000411843 in hmap_remove (node=0x7fd9d402e730,
>>> hmap=0x1c3e9e8)
>>>
>>> at lib/hmap.h:236
>>>
>>> #1 update_recirc_rules (bond=0x1c3e920) at ofproto/bond.c:382
>>>
>>> #2 0x0000000000001ff5 in ?? ()
>>>
>>> (gdb) p *hmap
>>>
>>> $1 = {buckets = 0x7fd9d4039b70, one = 0x0, mask = 255, n = 147}
>>>
>>> (gdb) p *node
>>>
>>> $2 = {hash = 2450845263, next = 0x0}
>>>
>>> (gdb) p *bucket
>>>
>>> Cannot access memory at address 0x8
>>>
>>> (gdb) p *(hmap->buckets)
>>>
>>> $3 = (struct hmap_node *) 0x0
>>>
>>> (gdb) p node->hash & hmap->mask
>>>
>>> $4 = 79
>>>
>>> (gdb) p *(&hmap->buckets[79])
>>>
>>> $5 = (struct hmap_node *) 0x0
>>>
>>> I managed to reproduce the error using a while loop in which I repeatedly
>>> create and destroy a lacp bond, waiting for 15 seconds after each of
>>> these
>>> operations.
>>>
>>> The error is not constant in timing and I think that it can be a race
>>> condition due to the recirculation process. It happens indeed that when
>>> calling hmap_remove() from update_recirc_rules() (in case of DEL), the
>>> bucket points to NULL. My idea is the following:
>>>
>>> hmap_remove() is called from two different places in ofproto/bond.c:
>>>
>>> 1. from update_recirc_rules() (in which it raises the segfault)
>>>
>>> hmap_remove(&bond->pr_rule_ops, &pr_op->hmap_node);
>>> *pr_op->pr_rule = NULL;
>>> free(pr_op);
>>>
>>> 2. and from bond_unref() (called when I try to delete the bond)
>>>
>>> HMAP_FOR_EACH_SAFE(pr_op, next_op, hmap_node, &bond->pr_rule_ops) {
>>> hmap_remove(&bond->pr_rule_ops, &pr_op->hmap_node);
>>> free(pr_op);
>>> }
>>>
>>> Now, if these two calls happen at the same time, a conflict on the bond
>>> pr_rule may happen. Looking at the code I can see that the recirculation
>>> hmap_remove() is executed inside an external lock
>>> (ovs_rwlock_wrlock(&rwlock)) if called by bond_rebalance()
>>>
>>> void bond_rebalance(struct bond *bond) {
>>> ...
>>> ovs_rwlock_wrlock(&rwlock);
>>> ...
>>> ...
>>> if (use_recirc && rebalanced) {
>>> bond_update_post_recirc_rules(bond,true); <---- hmap_remove()
>>> }
>>>
>>> done:
>>> ovs_rwlock_unlock(&rwlock);
>>> }
>>>
>>> but I can't see any lock when called by output_normal()
>>>
>>> static void output_normal(struct xlate_ctx *ctx, const struct xbundle
>>> *out_xbundle, uint16_t vlan) {
>>> ...
>>> if (ctx->use_recirc) {
>>> ...
>>> bond_update_post_recirc_rules(out_xbundle->bond, false); <----
>>> hmap_remove()
>>> ...
>>> }
>>> ...
>>>
>>> The same lock is used in bond_unref() but only for hmap_remove(all_bonds,
>>> &bond->hmap_node) and not for hmap_remove(&bond->pr_rule_ops,
>>> &pr_op->hmap_node).
>>>
>>> ...
>>> ovs_rwlock_wrlock(&rwlock);
>>> hmap_remove(all_bonds, &bond->hmap_node);
>>> ovs_rwlock_unlock(&rwlock);
>>> ...
>>> HMAP_FOR_EACH_SAFE(pr_op, next_op, hmap_node, &bond->pr_rule_ops) {
>>> hmap_remove(&bond->pr_rule_ops, &pr_op->hmap_node);
>>> free(pr_op);
>>> }
>>> ...
>>>
>>> I suppose that the goal is to remove any pointer to the bond, from
>>> all_bonds, inside the lock and to work on it locally. My doubt is: can
>>> the
>>> bond be still accessible from somewhere else (let's say from a bundle)?
>>> If
>>> yes, what happen if a bundle tries to access a bond which was previously
>>> removed from all_bonds (let's say from bundle_destroy())?
>>>
>>> The version of OVS that I am using is openvswitch-2.3.0-7.8312.x86_64.
>>>
>>> Could you please help me to find the problem?
>>>
>>>
>>> Thank you for the help & kind regards,
>>>
>>> Salvatore
>>>
>>>
>>>
>>> _______________________________________________
>>> discuss mailing list
>>> disc...@openvswitch.org
>>> http://openvswitch.org/mailman/listinfo/discuss
>>>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
