On 05/29/2012 02:21 PM, Vasanthakumar Thiagarajan wrote:
> On Tuesday 29 May 2012 04:43 PM, Kalle Valo wrote:
>> On 05/25/2012 01:19 PM, Vasanthakumar Thiagarajan wrote:
>>
>>> @@ -1188,6 +1189,7 @@ static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid,
>>> rxtid->progress = true;
>>> else
>>> for (idx = 0 ; idx< rxtid->hold_q_sz; idx++) {
>>> + spin_lock_bh(&rxtid->lock);
>>> if (rxtid->hold_q[idx].skb) {
>>> /*
>>> * There is a frame in the queue and no
>>> @@ -1201,8 +1203,10 @@ static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid,
>>> HZ * (AGGR_RX_TIMEOUT) / 1000));
>>> rxtid->progress = false;
>>> rxtid->timer_mon = true;
>>> + spin_unlock_bh(&rxtid->lock);
>>> break;
>>> }
>>> + spin_unlock_bh(&rxtid->lock);
>>> }
>>
>> Here you take and release the lock multiple times inside the loop. Why
>> not take the lock before the loop?
>
> Sounds better to acquire the lock before loop and releasing it after
> the loop. I was kind of thinking about protecting the data only
> in critical section, but in this case we can bring the loop
> in the lock, it is not going to make much difference.
It's also the question of does the lock protect hold_q_sz.
Also, can you please fix the comment I added to core.h about the lock
and document properly what the lock is actually supposed to protect:
/*
* FIXME: No clue what this should protect. Apparently it should
* protect some of the fields above but they are also accessed
* without taking the lock.
*/
> I'll change this.
Thanks.
Kalle
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
