On 18 Nov 2013, at 11:59 pm, Martin Pieuchot <mpieuc...@nolizard.org> wrote:
> 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™ :) yes there is. task_del returns 1 if the task was taken off the list. if the task is on the list and task_del returns 1, then you know it wont run in the future. if it was on the list and returns 0, you know its currently running or has run. > 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. you are free to do that, but i dont see how that solves the problem. generally a task is doing work on something more than itself, ie, you would pass it a reference to something in a softc anyway so you still cant free the softc until you're sure the task is done or not going to run. except now you dont have a reference to the task so you cant task_del it outside the task handler. > >>> 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. this is naive. passing an index doesnt stop an ifp from being freed after you got a pointer to it with if_get() but while you're in the middle of using it in a task. no matter how you pass a handle to an ifp, either via an index or a direct pointer, you still need to lock or reference count access to it. > But this approach only makes sense if the task storage is not freed when > the interface is destroyed. once you task_del you know that the actual struct task memory isnt referenced anywhere and can be safely freed, just like timeouts. it makes no promises about what you passed to task_set though. > >>> 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. agreed. where do you want to talk about this stuff? dlg
