vhost: replace vhost_workqueue with per-vhost kthread
in [Kernel]
|
From: Michael S. Tsirkin on 26 Jul 2010 16:30 On Mon, Jul 26, 2010 at 09:04:17PM +0200, Tejun Heo wrote: > On 07/26/2010 06:23 PM, Michael S. Tsirkin wrote: > >> * Can you please keep the outer goto repeat loop? I just don't like > >> outermost for (;;). > > > > Okay ... can we put the code in a {} scope to make it clear > > where does the loop starts and ends? > > If we're gonna do that, it would be better to put it inside a loop > construct. The reason why I don't like it is that loops like that > don't really help read/writeability much while indenting the whole > logic unnecessarily and look more like a result of obsession against > goto rather than any practical reason. It's just a cosmetic > preference and I might as well be the weirdo here, so if you feel > strong about it, please feel free to put everything in a loop. > > >> * Placing try_to_freeze() could be a bit annoying. It shouldn't be > >> executed when there's a work to flush. > > > > It currently seems to be executed when there is work to flush. > > Is this wrong? > > Oh, does it? As I wrote in the other mail, things like that wouldn't > necessarily break correctness but I think it would be better to avoid > surprises in the generic code if not too difficult. Let's try to define what do we want to achieve then. Do you want code that flushes workers not to block when workers are frozen? How will we handle work submitted when worker is frozen? > >> * I think A - B <= 0 test would be more familiar. At least > >> time_before/after() are implemented that way. > > > > I am concerned that this overflows a signed integer - > > which I seem to remeber that C99 disallows. > > Really? Overflows of pointer isn't expected and that's why we have > weird RELOC_HIDE() macro for such calculations but integers not > expected to overflow is a news to me. Are you sure? That basically > means time_before/after() aren't safe either. As I said, in C99. However, the kernel is built with -fno-strict-overflow, so it will work. > > timer macros are on data path so might be worth the risk there, > > but flush is slow path so better be safe? > > I don't think performance matters much here. I just think the sign > test is clearer / more familiar for the logic. > > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Michael S. Tsirkin on 27 Jul 2010 15:30 On Mon, Jul 26, 2010 at 09:31:58PM +0200, Tejun Heo wrote: > Hello, > > On 07/26/2010 09:14 PM, Tejun Heo wrote: > > On 07/26/2010 06:51 PM, Michael S. Tsirkin wrote: > >> I noticed that with vhost, flush_work was getting the worker > >> pointer as well. Can we live with this API change? > > > > Yeah, the flushing mechanism wouldn't work reliably if the work is > > queued to a different worker without flushing, so yeah passing in > > @worker might actually be better. > > Thinking a bit more about it, it kind of sucks that queueing to > another worker from worker->func() breaks flush. Maybe the right > thing to do there is using atomic_t for done_seq? I don't believe it will help: we might have: worker1 runs work work requeues itself queued index = 1 worker1 reads queued index = 1 worker2 runs work work requeues itself queued index = 2 worker2 runs work worker2 reads queued index = 2 worker2 writes done index = 2 worker1 writes done index = 1 As you see, done index got moved back. > It pays a bit more > overhead but maybe that's justifiable to keep the API saner? It would > be great if it can be fixed somehow even if it means that the work has > to be separately flushed for each worker it has been on before being > destroyed. > > Or, if flushing has to be associated with a specific worker anyway, > maybe it would be better to move the sequence counter to > kthread_worker and do it similarly with the original workqueue so that > work can be destroyed once execution starts? Then, it can at least > remain semantically identical to the original workqueue. > > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Michael S. Tsirkin on 28 Jul 2010 07:00 On Wed, Jul 28, 2010 at 09:48:31AM +0200, Tejun Heo wrote: > On 07/27/2010 09:19 PM, Michael S. Tsirkin wrote: > >> Thinking a bit more about it, it kind of sucks that queueing to > >> another worker from worker->func() breaks flush. Maybe the right > >> thing to do there is using atomic_t for done_seq? > > > > I don't believe it will help: we might have: > > > > worker1 runs work > > work requeues itself queued index = 1 > > worker1 reads queued index = 1 > > worker2 runs work > > work requeues itself queued index = 2 > > worker2 runs work > > worker2 reads queued index = 2 > > worker2 writes done index = 2 > > worker1 writes done index = 1 > > > > As you see, done index got moved back. > > Yeah, I think the flushing logic should be moved to the worker. > Are you interested in doing it w/ your change? > > Thanks. I'm unsure how flush_work operates under these conditions. E.g. in workqueue.c, this seems to work by keeping a pointer to current workqueue in the work. But what prevents us from destroying the workqueue when work might not be running? Is this currently broken if you use multiple workqueues for the same work? If yes, I propose we do as I did, making flush_work get worker pointer, and only flushing on that worker. > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
First
|
Prev
|
Pages: 1 2 3 Prev: firewire: new driver: nosy - IEEE 1394 traffic sniffer Next: Small perf optimization |