Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 17 Aug 2012 10:53:20 +0200
From:      Bernhard Schmidt <bschmidt@techwires.net>
To:        PseudoCylon <moonlightakkiy@yahoo.ca>
Cc:        freebsd-wireless@freebsd.org, Kim Culhan <w8hdkim@gmail.com>
Subject:   Re: (ANother) stall fixed, please update to HEAD
Message-ID:  <CAAgh0_a6i50UBLJ9GqKY4a9Ttgd99Xpw3OiVJvqwKNjF73yOAg@mail.gmail.com>
In-Reply-To: <CAFZ_MYL%2BPJp=fZGY3HUuL%2BLtMGh1QaAMrEZsoig5sKoV=ESs=w@mail.gmail.com>
References:  <CAFZ_MYL%2BPJp=fZGY3HUuL%2BLtMGh1QaAMrEZsoig5sKoV=ESs=w@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Aug 9, 2012 at 11:32 AM, PseudoCylon <moonlightakkiy@yahoo.ca> wrote:
>> ------------------------------
>>
>> Message: 5
>> Date: Tue, 7 Aug 2012 12:34:52 -0400
>> From: Kim Culhan <w8hdkim@gmail.com>
>> Subject: Re: (ANother) stall fixed, please update to HEAD
>> To: Adrian Chadd <adrian.chadd@gmail.com>
>> Cc: freebsd-wireless@freebsd.org
>> Message-ID:
>>         <CAKZxVQVbKfW7WVuZjNFg5pGFu3Djzz=60FoSNTS83sz1zf72wQ@mail.gmail.com>
>> Content-Type: text/plain; charset=ISO-8859-1
>>
>>
>>> Yup. Would you be able to work with PseudoCylon and test his
>>> ieee80211_iterate_nodes() patch? I'd like verification that it fixes
>>> it for you before I tidy it up and commit it to -HEAD.
>>
>> iter.patch and iter2.patch cannot be applied in that order
>> soo..  PseudoCylon could you please generate a diff
>> against -HEAD from your present local source?
>>
>
> No it won't. Only one of them need to apply. I guess I didn't explain well.
>
> iter.path only print outs debug message when the array overflowed
> (most unlikely it will). At this moment, this is what we need.
>
> iter2.patch does iter.patch + revert changes + abort iterating just
> for piece of mind. Probably this is unnecessary. The code need to be
> patched in the way the array won't overflow if it ever happens. (I
> leave it to committers what to commit.)
>
> The attached patch can be applied over iter.patch
>
> Sorry for the confusion.

Sorry for jumping in that late, but I've a bit of a bellyache with
that change especially the one which got committed.

You've turned a totally safe function call into one which can fail,
and malloc will fail eventually, therefore all callers need to either
handle the error case or need to clearly state that it is not
necessary to do something. I have the feeling that you've introduced
some cases where due to lost calls the whole system might be hanging
(not as hard as a deadlock, but hard enough to require manual
intervention) by not doing a state change or something.

Basically, all you've tried to achieve here is an additional unlock to
avoid a deadlock when the called function itself needs to acquire the
lock, performance aside, I'd really prefer if we're simply adding the
unlock and go back to the "restart" approach. At least until someone
either fixes the callers or can come up with a safe enough solution.

Thanks

-- 
Bernhard



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAAgh0_a6i50UBLJ9GqKY4a9Ttgd99Xpw3OiVJvqwKNjF73yOAg>