On 15/11/2019 07:26, Hannes Reinecke wrote:

On 11/14/19 10:41 AM, John Garry wrote:

quoted

On 13/11/2019 18:38, Hannes Reinecke wrote:

quoted

quoted

Hi Hannes,

quoted

Oh, my. Indeed, that's correct.

The tags could be kept in sync like this:

shared_tag = blk_mq_get_tag(shared_tagset);
if (shared_tag != -1)
      sbitmap_set(hctx->tags, shared_tag);

But that's obviously not ideal.

Actually, I _do_ prefer keeping both in sync.
We might want to check if the 'normal' tag is set (typically it would
not, but then, who knows ...)
The beauty here is that both 'shared' and 'normal' tag are in sync, so
if a driver would be wanting to use the tag as index into a command
array it can do so without any surprises.

Why do you think it's not ideal?

A few points:
- Getting a bit from one tagset and then setting it in another tagset is
a bit clunky.

Yes, that's true.
But painstakingly trying to find a free bit in a bitmask when we already
know which to pick is also a bit daft.

Yeah, but it's not all good - there would still be a certain overhead in 
the atomic set and unset bit on the hctx sbitmap. However it still 
should be faster as it excludes the search.

quoted

- There may be an atomicity of the getting the shared tag bit and
setting the hctx tag bit - I don't think that there is.

That was precisely what I've alluded to in 'We might want to check if
the normal tag is set'.
Typically the 'normal' tag would be free (as the shared tag set out of
necessity needs to be the combination of all hctx tag sets).

Right

Any difference here _is_ a programming error, and should be flagged as
such (sbitmap_test_and_set() anyone?)

Eh, I hope that we wouldn't need this.

We might have ordering issues on release, as we really should drop the
hctx tag before the shared tag; but when we observe that we should be fine.

quoted

- Consider that sometimes we may want to check if there is space on a hw
queue - checking the hctx tags is not really proper any longer, as
typically there would always be space on hctx, but not always the shared
tags. We did delete blk_mq_can_queue() yesterday, which would be an
example of that. Need to check if there are others.

Clearly, this needs an audit of all functions accessing the hctx tag
space; maybe it's worth having a pre-requisite patchset differentiating
between hctx tags and global, shared tags. Hmm.

quoted

Having said all that, the obvious advantage is performance gain, can
still use request.tag and so maybe less intrusive changes.

I'll have a look at the implementation. The devil is mostly in the
detail...

True.
And, incidentally, if we run with shared tage we can skip the scheduling
section in blk_mq_get_tag(); if we're out of tags, we're out of tags,

Right, but don't we need to then "kick all hw queues", instead of just 
that for the current hctx in blk_mq_get_tag() when free tags are exhausted?

and no rescheduling will help as we don't _have_ other tagsets to look at.

But overall I like this approach.

Yeah, to me it seems sensible. Again, a neat implementation is the 
challenge.

I'll post an RFC v2 for this one.

I am also requesting some performance figures also internally.

Thanks,
John