[PATCH] media: add GFP flag to media_*() that could get called in atomic context
Mauro Carvalho Chehab
mchehab at osg.samsung.com
Tue Mar 15 15:55:35 UTC 2016
Em Mon, 14 Mar 2016 14:09:09 +0200
Sakari Ailus <sakari.ailus at iki.fi> escreveu:
> Hi Mauro,
>
> On Mon, Mar 14, 2016 at 08:46:33AM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 14 Mar 2016 12:52:54 +0200
> > Sakari Ailus <sakari.ailus at iki.fi> escreveu:
> >
> > > Hi Mauro,
> > >
> > > On Mon, Mar 14, 2016 at 07:13:58AM -0300, Mauro Carvalho Chehab wrote:
> > > > Em Mon, 14 Mar 2016 09:22:37 +0200
> > > > Sakari Ailus <sakari.ailus at iki.fi> escreveu:
> > > >
> > > > > Hi Shuah,
> > > > >
> > > > > On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:
> > > > > > Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> > > > > > media_devnode_create(), and media_add_link() that could get called
> > > > > > in atomic context to allow callers to pass in the right flags for
> > > > > > memory allocation.
> > > > > >
> > > > > > tree-wide driver changes for media_*() GFP flags change:
> > > > > > Change drivers to add gfpflags to interffaces, media_create_pad_link(),
> > > > > > media_create_intf_link() and media_devnode_create().
> > > > > >
> > > > > > Signed-off-by: Shuah Khan <shuahkh at osg.samsung.com>
> > > > > > Suggested-by: Mauro Carvalho Chehab <mchehab at osg.samsung.com>
> > > > >
> > > > > What's the use case for calling the above functions in an atomic context?
> > > > >
> > > >
> > > > ALSA code seems to do a lot of stuff at atomic context. That's what
> > > > happens on my test machine when au0828 gets probed before
> > > > snd-usb-audio:
> > > > http://pastebin.com/LEX5LD5K
> > > >
> > > > It seems that ALSA USB probe routine (usb_audio_probe) happens in
> > > > atomic context.
> > >
> > > usb_audio_probe() grabs a mutex (register_mutex) on its own. It certainly
> > > cannot be called in atomic context.
> > >
> > > In the above log, what I did notice, though, was that because *we* grab
> > > mdev->lock spinlock in media_device_register_entity(), we may not sleep
> > > which is what the notify() callback implementation in au0828 driver does
> > > (for memory allocation).
> >
> > True. After looking into the code, the problem is that the notify
> > callbacks are called with the spinlock hold. I don't see any reason
> > to do that.
>
> Notify callbacks, perhaps not, but the list is still protected by the
> spinlock. It perhaps is not likely that another process would change it but
> I don't think we can rely on that.
I can see only 2 risks protected by the lock:
1) mdev gets freed while an entity is being created. This is a problem
with the current memory protection schema we're using. I guess the
only way to fix it is to use kref for mdev/entities/interfaces/links/pads.
This change doesn't make it better or worse.
Also, I don't think we have such risk with the current devices.
2) a notifier may be inserted or removed by another driver, while the
loop is running.
To avoid (2), I see 3 alternatives:
a) keep the loop as proposed on this patch. As the list is navigated using
list_for_each_entry_safe(), I guess[1] it should be safe to remove/add
new notify callbacks there while the loop is running by some other process.
[1] It *is* safe if the change were done inside the loop - but I'm not
100% sure that it is safe if some other CPU touches the notify list.
b) Unlock/relock the spinlock every time:
/* previous code that locks mdev->lock spinlock */
/* invoke entity_notify callbacks */
list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) {
spin_unlock(&mdev->lock);
(notify)->notify(entity, notify->notify_data);
spin_lock(&mdev->lock);
}
spin_unlock(&mdev->lock);
c) use a separate lock for the notify list -this seems to be an overkill.
d) Protect it with the graph traversal mutex. That sounds the worse idea,
IMHO, as we'll be abusing the lock.
>
> >
> > > Could we instead replace mdev->lock by a mutex?
> >
> > We changed the code to use a spinlock for a reason: this fixed some
> > troubles in the past with the code locking (can't remember the problem,
> > but this was documented at the kernel logs and at the ML). Yet, the code
> > under the spinlock never sleeps, so this is fine.
>
> struct media_device.lock was added by this patch:
>
> commit 53e269c102fbaf77e7dc526b1606ad4a48e57200
> Author: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Date: Wed Dec 9 08:40:00 2009 -0300
>
> [media] media: Entities, pads and links
>
> As video hardware pipelines become increasingly complex and
> configurable, the current hardware description through v4l2 subdevices
> reaches its limits. In addition to enumerating and configuring
> subdevices, video camera drivers need a way to discover and modify at
> runtime how those subdevices are connected. This is done through new
> elements called entities, pads and links.
>
> ...
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Sakari Ailus <sakari.ailus at iki.fi>
> Acked-by: Hans Verkuil <hverkuil at xs4all.nl>
> Signed-off-by: Mauro Carvalho Chehab <mchehab at redhat.com>
>
> I think it was always a spinlock, for the reason you stated above as well:
> it did not need to sleep. But if there is a need to sleep, I think we should
> consider changing that.
True, but there were some places that were using the graph_mutex
instead of the spinlock.
>
> >
> > Yet, in the future, we'll need to do a review of all the locking schema,
> > in order to better support dynamic graph changes.
>
> Agreed. I think more fine grained locking should be considered. The media
> graph mutex will become a bottleneck at some point, especially if we make
> the media devices system wide at some point.
Yes. I guess we should protect the memory allocated stuff with a kref,
and try to use RCS on most places, but we need more discussions and
more tests to implement a solution that would be reliable even if the
callers don't behave well.
> >
> > > If there is no pressing need to implement atomic memory allocation I simply
> > > wouldn't do it, especially in device initialisation where an allocation
> > > failure will lead to probe failure as well.
> >
> > The fix for this issue should be simple. See the enclosed code. Btw.
> > it probably makes sense to add some code here to avoid starving the
> > stack, as a notify callback could try to create an entity, with,
> > in turn, would call the notify callback again.
> >
> > I'll run some tests here to double check if it fixes the issue.
> >
> > ---
> >
> > [media] media-device: Don't call notify callbacks holding a spinlock
> >
> > The notify routines may sleep. So, they can't be called in spinlock
> > context. Also, they may want to call other media routines that might
> > be spinning the spinlock, like creating a link.
> >
> > This fixes the following bug:
> >
> > [ 1839.510587] BUG: sleeping function called from invalid context at mm/slub.c:1289
> > [ 1839.510881] in_atomic(): 1, irqs_disabled(): 0, pid: 3479, name: modprobe
> > [ 1839.511157] 4 locks held by modprobe/3479:
> > [ 1839.511415] #0: (&dev->mutex){......}, at: [<ffffffff81ce8933>] __driver_attach+0xa3/0x160
> > [ 1839.512381] #1: (&dev->mutex){......}, at: [<ffffffff81ce8941>] __driver_attach+0xb1/0x160
> > [ 1839.512388] #2: (register_mutex#5){+.+.+.}, at: [<ffffffffa10596c7>] usb_audio_probe+0x257/0x1c90 [snd_usb_audio]
> > [ 1839.512401] #3: (&(&mdev->lock)->rlock){+.+.+.}, at: [<ffffffffa0e6051b>] media_device_register_entity+0x1cb/0x700 [media]
> > [ 1839.512412] CPU: 2 PID: 3479 Comm: modprobe Not tainted 4.5.0-rc3+ #49
> > [ 1839.512415] Hardware name: /NUC5i7RYB, BIOS RYBDWi35.86A.0350.2015.0812.1722 08/12/2015
> > [ 1839.512417] 0000000000000000 ffff8803b3f6f288 ffffffff81933901 ffff8803c4bae000
> > [ 1839.512422] ffff8803c4bae5c8 ffff8803b3f6f2b0 ffffffff811c6af5 ffff8803c4bae000
> > [ 1839.512427] ffffffff8285d7f6 0000000000000509 ffff8803b3f6f2f0 ffffffff811c6ce5
> > [ 1839.512432] Call Trace:
> > [ 1839.512436] [<ffffffff81933901>] dump_stack+0x85/0xc4
> > [ 1839.512440] [<ffffffff811c6af5>] ___might_sleep+0x245/0x3a0
> > [ 1839.512443] [<ffffffff811c6ce5>] __might_sleep+0x95/0x1a0
> > [ 1839.512446] [<ffffffff8155aade>] kmem_cache_alloc_trace+0x20e/0x300
> > [ 1839.512451] [<ffffffffa0e66e3d>] ? media_add_link+0x4d/0x140 [media]
> > [ 1839.512455] [<ffffffffa0e66e3d>] media_add_link+0x4d/0x140 [media]
> > [ 1839.512459] [<ffffffffa0e69931>] media_create_pad_link+0xa1/0x600 [media]
> > [ 1839.512463] [<ffffffffa0fe11b3>] au0828_media_graph_notify+0x173/0x360 [au0828]
> > [ 1839.512467] [<ffffffffa0e68a6a>] ? media_gobj_create+0x1ba/0x480 [media]
> > [ 1839.512471] [<ffffffffa0e606fb>] media_device_register_entity+0x3ab/0x700 [media]
> >
> > (untested)
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab at osg.samsung.com>
> >
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 6ba6e8f982fc..fc3c199e5500 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -587,14 +587,15 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> > media_gobj_create(mdev, MEDIA_GRAPH_PAD,
> > &entity->pads[i].graph_obj);
> >
> > + spin_unlock(&mdev->lock);
> > +
> > /* invoke entity_notify callbacks */
> > list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) {
> > (notify)->notify(entity, notify->notify_data);
> > }
> >
> > - spin_unlock(&mdev->lock);
> > -
> > mutex_lock(&mdev->graph_mutex);
> > +
> > if (mdev->entity_internal_idx_max
> > >= mdev->pm_count_walk.ent_enum.idx_max) {
> > struct media_entity_graph new = { .top = 0 };
> >
>
--
Thanks,
Mauro
More information about the devel
mailing list