Re: [BUG] act_ife: sleeping functions called in atomic context
From: Cong Wang
Date: Fri Jun 17 2016 - 13:32:06 EST
On Fri, Jun 17, 2016 at 4:07 AM, Jamal Hadi Salim <jhs@xxxxxxxxxxxx> wrote:
> On 16-06-17 01:38 AM, Cong Wang wrote:
>>
>> On Thu, Jun 16, 2016 at 7:14 PM, Cong Wang <xiyou.wangcong@xxxxxxxxx>
>> wrote:
>>>
>>>
>>> I think we can just remove that tcf_lock, I am testing a patch now.
>>
>>
>> Please try the attached patch, I will do more tests tomorrow.
>>
>> Thanks!
>>
>
> Cong, What tree are you using? I dont see the time aggregation patches
> that I sent (and Dave took in) in your changes.
My patch is against -net. (I see you already figured out your patch is
missing in -net-next.)
Or are you suggesting to rebase it for -net-next? I think it fixes some real
bug so -net is better, although it is slightly large as a bug fix.
>
> Comments:
> Is GFP_ATOMIC really necessary? Thats user->kernel interface. GFP_KERNEL
> should be sufficient.
I added a read_lock(ife_mod_lock), this is why we need
GFP_ATOMIC.
Again, don't worry, this change should be in a separated patch,
you will not miss it again when I send them formally. ;)
> Also, it would be nice to kill the lock - but this feels like two
> patches in one. 1) to fix the alloc not to be under the lock 2) to
> kill said lock. Maybe split it as such for easier review.
> I am using this action extensively so will be happy to test.
> I think my patch is a good beginning to #1 - if you fix the forgotten
> unlock and ensure we lock around updating ife fields when it exists
> already (you said it in your earlier email and I thought about
> that afterwards).
Yes, it makes sense too. Your patch is smaller, if you plan to
backport it to stable, we can use your patch for -net and -stable
and I am happy to rebase mine for -net-next.
I am fine with either way.
Thanks!