Em Mon, 14 Mar 2016 14:09:09 +0200 Sakari Ailus <sakari.ailus@xxxxxx> 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@xxxxxx> 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@xxxxxx> 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@xxxxxxxxxxxxxxx> > > > > > > Suggested-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx> > > > > > > > > > > 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@xxxxxxxxxxxxxxxx> > 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@xxxxxxxxxxxxxxxx> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxx> > Acked-by: Hans Verkuil <hverkuil@xxxxxxxxx> > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx> > > > > 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
