On 18/11/13(Mon) 13:35, Stefan Sperling wrote: > On Mon, Nov 18, 2013 at 12:37:53PM +0100, Martin Pieuchot wrote: > > Even if right now calling task_del() is enough, do you know if there's > > an easy way to convert this code without putting the task storage in > > the chunk of memory it manipulates? In other words having the "struct > > task" outside of the softc? > > Well, tasks are stored in softc everywhere I looked for examples.
I know :) But how many of these softc can be freed? I also introduced a similar situation in audio(4) if it is attached to uaudio(4), and I'm looking for an alternative way of doing it. > The task storage and the storage for the task's arguments needs to > be known when task_set() is called. As I understand, task_set() is > called once, at attach time. Then the task gets scheduled or > cancelled with task_add()/task_del(), and when the task runs it gets > its arguments from the storage given to task_set(). > > If we wanted to store the task and its arguments outside the softc, > I guess we'd need to malloc() task memory at attach time and free it > on detach(). I.e. it has the same lifetime as the softc. So I don't > see the point of uncoupling it from the softc. The point is that there's no way to check if the task queue still has a reference to your task when you free() your softc, right now big lock is protecting us™ :) > Or we would need to call task_set() and task_add() every time we want > to schedule the task. I don't think that's what the tasqk API is intending. > > Or we would have the task itself free its own storage (task and > arguments), instead of freeing it when the softc is destroyed. > But I don't think that's what the API is intending either. That's why I'm asking, maybe you or somebody else already has an idea. > > I'm asking because if you can do it, this will make the task totally > > independent and won't require any task_del(). The idea behind that > > is that tomorrow we will try to have more kernel threads running in > > parallel, and if your task is about to run or already running when > > your interface is destroyed you might end up freeing the memory the > > task is manipulating. > > I'm assuming the task won't be interrupted in a way that would make the > ifp storage go away while it runs. Perhaps in a new world with more > kernel threads that won't hold. But that would cause quite a lot of > problems everywhere, not just sppp(4). > > When the ifp goes away, the task must be not be allowed to run if already > scheduled. Because a workq task cannot be cancelled, the bug where the > interface is deleted before the workq task gets to run is present in the > current code and fixed by switching to taskq, with the task_del() call > at interface detach time. How can we fix this bug without task_del()? > Would you have the task itself cope with an ifp that has disappeared? Yep, that's another way to fix this problem. You can pass the index of the interface to the task and call if_get() when it runs. If the interface is gone don't do anything otherwise run the task. But this approach only makes sense if the task storage is not freed when the interface is destroyed. > > Since the current code is already using allocating some memory for the > > arguments of the task, I'd argue that this is better than putting the > > task storage on the same memory chunk that it manipulates. But since > > this problem exists in other places in our tree, we might think of an > > alternative solution/api/whatever. > > For now, I'd like to stick with the "task in softc" idiom I saw elsewhere. Fine with me. > Generally, I think your concern is out of scope for my sppp(4) changes > and should be discussed in a separate discussion thread because using > a different idiom would affect many drivers. It is, clearly, but maybe you had an "easy way" to avoid it, apparently not ;) To be clear, my concern is only about drivers that can be destroyed/unplugged, that's hopefully not affecting so many drivers.
