> On 7 Dec 2015, at 11:34 PM, Martin Pieuchot <m...@openbsd.org> wrote: > > On 07/12/15(Mon) 19:37, David Gwynne wrote: >> On Sun, Dec 06, 2015 at 03:51:26PM +0100, Martin Pieuchot wrote: >>> On 06/12/15(Sun) 14:00, David Gwynne wrote: >>>> [...] >>>> the idea is you have a taskctx, which represents a serialising >>>> context for tasks. tasks are submitted to the taskctx, and the code >>>> will try to run the tasks immediately rather than defer them to a >>>> thread. if there is contention on the context, the contending cpu >>>> yields after queueing the task because the other cpu is responsible >>>> for running all pending tasks to completion. >>>> >>>> it also simplifies the barrier operations a lot. >>>> >>>> the diff below implements a generic taskctx framework, and cuts the >>>> mpsafe if_start() implementation over to it. >>> >>> I'm not sure this should be implemented as a generic framework. I'd >>> suggest to keep it specific to if_start(). As you say above the least >>> worst mechanism we have is currently taskqs, but maybe this could/will >>> change? >> >> im in two minds about where the ctx code should sit. i obviously >> lean toward keeping it with taskqs, but i also cant come up with >> another use case for it. i also havent tried very hard to think of >> one. > > Even if you have another use case for it I find this generic framework > confusing. It is build on top of taskq, but it's not part of the taskq > API. So putting the documentation in the same manual just confuse me. > Your task API is simple and people use it easily. I think it should > stay like that. > > Now we need a way to serialize start and txeof and you came up with a > solution... > >> are you asking if taskqs will get better, or if something better >> than taskqs will come along? > > ...I'm just saying that the "implementation" of your serialisation > mechanism should not matter. I'm fine with having a context for tasks > but I'd suggest that this should be abstracted in a simple API instead > of offering a framework to build upon.
ill tweak it in the morning. > Could you make it such that you don't need a task in myx_softc? sure. the end extreme would be having an if_txeof() function in if.c that wraps a lot of this up. however, i dont want to dictate that start and txeof have to be serialised. if you're careful it is possible to operate both the start and txeof side of a ring concurrently. the only coordination necessary is around the oactive handling. if some drivers want to completely serialise both start and txeof they should be free to do, but if they want to make it as concurrent as possible then they should be able to what myx is doing and only coordinate oactive. ill tinker with it tomorrow though. > >>> I cannot understand what's happening by reading the myx(4) chunk itself. >>> So I believe the interface needs to be polished. Would it be possible >>> to implement this serialization without introducing if_restart()? >> >> that is by far my least favourite bit of this diff. in my defense, >> i wrote it while my mother was trying to have a conversation with >> me, so it didn't get my full attention. ive torn if_restart out and >> implemented it completely in myx below. >> >>>> myx is also changed to only clr oactive from within the taskctx >>>> serialiser, thereby avoiding the race, but keeps the bulk of txeof >>>> outside the serialiser so it can run concurrently with start. >>>> >>>> other nics are free to serialise start and txeof within the >>>> ifq_serializer if they want, or not, it is up to them. >>>> >>>> thoughts? tests? opinions on messy .h files? >>> >>> It appears to me that you have unrelated changes: if_enter/if_leave. >> >> oops, yes. i suck at juggling diffs. maybe we should call if.c >> full and split it up ;) > > I think we should start with that because the I'm afraid we're already > cooking spaghetti. > > My question is the barrier and mpsafe code is related to a queue or to > an interface? In other words does it make sense to serialize on a > context in the sending queue? What about multiple queues? it's per queue. ifnet and ifqueue are mixed together atm cos we've long had a 1:1 mapping between them in our stack. right now im trying to make the code mpsafe. keeping the api change simple means passing an ifp when an ifq is technically more correctly. making it work for multiple queues is something to do properly in the future. ill try to be more careful about this now to avoid confusion though by at least keeping the data structures grouped properly. dlg
