Message ID | 20191128020147.191893-1-weiyongjun1@huawei.com |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
Series | [-next] um: vector: use GFP_ATOMIC under spin lock | expand |
On 28/11/2019 02:01, Wei Yongjun wrote: > A spin lock is taken here so we should use GFP_ATOMIC. > > Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers") > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > --- > arch/um/drivers/vector_kern.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c > index 92617e16829e..6ff0065a271d 100644 > --- a/arch/um/drivers/vector_kern.c > +++ b/arch/um/drivers/vector_kern.c > @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev, > kfree(vp->bpf->filter); > vp->bpf->filter = NULL; > } else { > - vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL); > + vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC); > if (vp->bpf == NULL) { > netdev_err(dev, "failed to allocate memory for firmware\n"); > goto flash_fail; > @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev, > if (request_firmware(&fw, efl->data, &vdevice->pdev.dev)) > goto flash_fail; > > - vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_KERNEL); > + vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_ATOMIC); > if (!vp->bpf->filter) > goto free_buffer; > > > > Acked-by: Anton Ivanov <anton.ivanov@cambridgegreys.co.uk> > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um >
On Thu, Nov 28, 2019 at 02:01:47AM +0000, Wei Yongjun wrote: > A spin lock is taken here so we should use GFP_ATOMIC. > > Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers") > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > --- > arch/um/drivers/vector_kern.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c > index 92617e16829e..6ff0065a271d 100644 > --- a/arch/um/drivers/vector_kern.c > +++ b/arch/um/drivers/vector_kern.c > @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev, > kfree(vp->bpf->filter); > vp->bpf->filter = NULL; > } else { > - vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL); > + vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC); > if (vp->bpf == NULL) { > netdev_err(dev, "failed to allocate memory for firmware\n"); > goto flash_fail; > @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev, > if (request_firmware(&fw, efl->data, &vdevice->pdev.dev)) ^^^^^^^^^^^^^^^^ Is it really possible to call request_firmware() while holding a spin_lock? I was so sure that read from the disk. regards, dan carpenter > goto flash_fail; > > - vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_KERNEL); > + vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_ATOMIC); > if (!vp->bpf->filter) > goto free_buffer; > >
On 28/11/2019 08:06, Dan Carpenter wrote: > On Thu, Nov 28, 2019 at 02:01:47AM +0000, Wei Yongjun wrote: >> A spin lock is taken here so we should use GFP_ATOMIC. >> >> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers") >> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> >> --- >> arch/um/drivers/vector_kern.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c >> index 92617e16829e..6ff0065a271d 100644 >> --- a/arch/um/drivers/vector_kern.c >> +++ b/arch/um/drivers/vector_kern.c >> @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev, >> kfree(vp->bpf->filter); >> vp->bpf->filter = NULL; >> } else { >> - vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL); >> + vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC); >> if (vp->bpf == NULL) { >> netdev_err(dev, "failed to allocate memory for firmware\n"); >> goto flash_fail; >> @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev, >> if (request_firmware(&fw, efl->data, &vdevice->pdev.dev)) > ^^^^^^^^^^^^^^^^ > > Is it really possible to call request_firmware() while holding a > spin_lock? I was so sure that read from the disk. Works, I tested the patch quite a few times. > > regards, > dan carpenter > >> goto flash_fail; >> >> - vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_KERNEL); >> + vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_ATOMIC); >> if (!vp->bpf->filter) >> goto free_buffer; >> >> > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um >
On Thu, Nov 28, 2019 at 08:18:30AM +0000, Anton Ivanov wrote: > > > On 28/11/2019 08:06, Dan Carpenter wrote: > > On Thu, Nov 28, 2019 at 02:01:47AM +0000, Wei Yongjun wrote: > > > A spin lock is taken here so we should use GFP_ATOMIC. > > > > > > Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers") > > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > > --- > > > arch/um/drivers/vector_kern.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c > > > index 92617e16829e..6ff0065a271d 100644 > > > --- a/arch/um/drivers/vector_kern.c > > > +++ b/arch/um/drivers/vector_kern.c > > > @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev, > > > kfree(vp->bpf->filter); > > > vp->bpf->filter = NULL; > > > } else { > > > - vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL); > > > + vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC); > > > if (vp->bpf == NULL) { > > > netdev_err(dev, "failed to allocate memory for firmware\n"); > > > goto flash_fail; > > > @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev, > > > if (request_firmware(&fw, efl->data, &vdevice->pdev.dev)) > > ^^^^^^^^^^^^^^^^ > > > > Is it really possible to call request_firmware() while holding a > > spin_lock? I was so sure that read from the disk. > > Works, I tested the patch quite a few times. > Do you have CONFIG_DEBUG_ATOMIC_SLEEP enabled? The GFP_KERNEL calls should have triggered a warning if so. regards, dan carpenter
----- Ursprüngliche Mail ----- > Von: "anton ivanov" <anton.ivanov@cambridgegreys.com> > An: "Dan Carpenter" <dan.carpenter@oracle.com>, "Wei Yongjun" <weiyongjun1@huawei.com> > CC: "Song Liu" <songliubraving@fb.com>, "Daniel Borkmann" <daniel@iogearbox.net>, "kernel-janitors" > <kernel-janitors@vger.kernel.org>, "richard" <richard@nod.at>, "Jeff Dike" <jdike@addtoit.com>, "linux-um" > <linux-um@lists.infradead.org>, "Alexei Starovoitov" <ast@kernel.org>, "netdev" <netdev@vger.kernel.org>, > bpf@vger.kernel.org, "Martin KaFai Lau" <kafai@fb.com> > Gesendet: Donnerstag, 28. November 2019 09:18:30 > Betreff: Re: [PATCH -next] um: vector: use GFP_ATOMIC under spin lock > On 28/11/2019 08:06, Dan Carpenter wrote: >> On Thu, Nov 28, 2019 at 02:01:47AM +0000, Wei Yongjun wrote: >>> A spin lock is taken here so we should use GFP_ATOMIC. >>> >>> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers") >>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> >>> --- >>> arch/um/drivers/vector_kern.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c >>> index 92617e16829e..6ff0065a271d 100644 >>> --- a/arch/um/drivers/vector_kern.c >>> +++ b/arch/um/drivers/vector_kern.c >>> @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device >>> *dev, >>> kfree(vp->bpf->filter); >>> vp->bpf->filter = NULL; >>> } else { >>> - vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL); >>> + vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC); >>> if (vp->bpf == NULL) { >>> netdev_err(dev, "failed to allocate memory for firmware\n"); >>> goto flash_fail; >>> @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device >>> *dev, >>> if (request_firmware(&fw, efl->data, &vdevice->pdev.dev)) >> ^^^^^^^^^^^^^^^^ >> >> Is it really possible to call request_firmware() while holding a >> spin_lock? I was so sure that read from the disk. > > Works, I tested the patch quite a few times. It works because of the nature of UML ->no SMP or PREEMPT. But better request the firmware before taking the spinlock. request_firmware() can block. Same for the kmalloc(), just allocate the buffer before and then assign the pointer under the lock. That way you don't need GFP_ATOMIC. Thanks, //richard
On 28/11/2019 08:37, Dan Carpenter wrote: > On Thu, Nov 28, 2019 at 08:18:30AM +0000, Anton Ivanov wrote: >> >> >> On 28/11/2019 08:06, Dan Carpenter wrote: >>> On Thu, Nov 28, 2019 at 02:01:47AM +0000, Wei Yongjun wrote: >>>> A spin lock is taken here so we should use GFP_ATOMIC. >>>> >>>> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers") >>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> >>>> --- >>>> arch/um/drivers/vector_kern.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c >>>> index 92617e16829e..6ff0065a271d 100644 >>>> --- a/arch/um/drivers/vector_kern.c >>>> +++ b/arch/um/drivers/vector_kern.c >>>> @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev, >>>> kfree(vp->bpf->filter); >>>> vp->bpf->filter = NULL; >>>> } else { >>>> - vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL); >>>> + vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC); >>>> if (vp->bpf == NULL) { >>>> netdev_err(dev, "failed to allocate memory for firmware\n"); >>>> goto flash_fail; >>>> @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev, >>>> if (request_firmware(&fw, efl->data, &vdevice->pdev.dev)) >>> ^^^^^^^^^^^^^^^^ >>> >>> Is it really possible to call request_firmware() while holding a >>> spin_lock? I was so sure that read from the disk. >> >> Works, I tested the patch quite a few times. >> > > Do you have CONFIG_DEBUG_ATOMIC_SLEEP enabled? The GFP_KERNEL calls > should have triggered a warning if so. I do not think we can use that in um. config DEBUG_ATOMIC_SLEEP bool "Sleep inside atomic section checking" select PREEMPT_COUNT depends on DEBUG_KERNEL depends on !ARCH_NO_PREEMPT In arch/um/Kconfig select ARCH_NO_PREEMPT Brgds, > > regards, > dan carpenter > > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um >
On 28/11/2019 08:41, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- >> Von: "anton ivanov" <anton.ivanov@cambridgegreys.com> >> An: "Dan Carpenter" <dan.carpenter@oracle.com>, "Wei Yongjun" <weiyongjun1@huawei.com> >> CC: "Song Liu" <songliubraving@fb.com>, "Daniel Borkmann" <daniel@iogearbox.net>, "kernel-janitors" >> <kernel-janitors@vger.kernel.org>, "richard" <richard@nod.at>, "Jeff Dike" <jdike@addtoit.com>, "linux-um" >> <linux-um@lists.infradead.org>, "Alexei Starovoitov" <ast@kernel.org>, "netdev" <netdev@vger.kernel.org>, >> bpf@vger.kernel.org, "Martin KaFai Lau" <kafai@fb.com> >> Gesendet: Donnerstag, 28. November 2019 09:18:30 >> Betreff: Re: [PATCH -next] um: vector: use GFP_ATOMIC under spin lock > >> On 28/11/2019 08:06, Dan Carpenter wrote: >>> On Thu, Nov 28, 2019 at 02:01:47AM +0000, Wei Yongjun wrote: >>>> A spin lock is taken here so we should use GFP_ATOMIC. >>>> >>>> Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers") >>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> >>>> --- >>>> arch/um/drivers/vector_kern.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c >>>> index 92617e16829e..6ff0065a271d 100644 >>>> --- a/arch/um/drivers/vector_kern.c >>>> +++ b/arch/um/drivers/vector_kern.c >>>> @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device >>>> *dev, >>>> kfree(vp->bpf->filter); >>>> vp->bpf->filter = NULL; >>>> } else { >>>> - vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL); >>>> + vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC); >>>> if (vp->bpf == NULL) { >>>> netdev_err(dev, "failed to allocate memory for firmware\n"); >>>> goto flash_fail; >>>> @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device >>>> *dev, >>>> if (request_firmware(&fw, efl->data, &vdevice->pdev.dev)) >>> ^^^^^^^^^^^^^^^^ >>> >>> Is it really possible to call request_firmware() while holding a >>> spin_lock? I was so sure that read from the disk. >> >> Works, I tested the patch quite a few times. > > It works because of the nature of UML ->no SMP or PREEMPT. > But better request the firmware before taking the spinlock. > request_firmware() can block. > Same for the kmalloc(), just allocate the buffer before and then assign > the pointer under the lock. That way you don't need GFP_ATOMIC. Ack. I will make an incremental on top of the existing patch (as that is already in -next Brgds, > > Thanks, > //richard > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um >
diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c index 92617e16829e..6ff0065a271d 100644 --- a/arch/um/drivers/vector_kern.c +++ b/arch/um/drivers/vector_kern.c @@ -1402,7 +1402,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev, kfree(vp->bpf->filter); vp->bpf->filter = NULL; } else { - vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_KERNEL); + vp->bpf = kmalloc(sizeof(struct sock_fprog), GFP_ATOMIC); if (vp->bpf == NULL) { netdev_err(dev, "failed to allocate memory for firmware\n"); goto flash_fail; @@ -1414,7 +1414,7 @@ static int vector_net_load_bpf_flash(struct net_device *dev, if (request_firmware(&fw, efl->data, &vdevice->pdev.dev)) goto flash_fail; - vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_KERNEL); + vp->bpf->filter = kmemdup(fw->data, fw->size, GFP_ATOMIC); if (!vp->bpf->filter) goto free_buffer;
A spin lock is taken here so we should use GFP_ATOMIC. Fixes: 9807019a62dc ("um: Loadable BPF "Firmware" for vector drivers") Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> --- arch/um/drivers/vector_kern.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)