On 11/29/2015 10:32 PM, Max Khon wrote:
Hello!
Attached patch implements random allocator for TCMU data area.
Any comments are appreciated!
Hi Max, glad to see your work!
Please next time inline the patch into the text of the email.
The only question I have is how can I handle timed-out commands (see
XXX: comment in free_data_area())? There is the following comment in
"struct tcmu_cmd" definition:
/* Can't use se_cmd->data_length when cleaning up expired
cmds, because if
cmd has been completed then accessing se_cmd is off limits */
Can you clarify why is that?
See tcmu_check_expired_cmd. If the command has timed out then we've
called target_complete_cmd already and the fabric may have already freed
the se_cmd. We clear the se_cmd pointer to make sure we don't use a
stale pointer, and therefore need our own copy of data_length in
tcmu_cmd in order to know how much space in the data ring was being used
by the timed-out command.
Concerning this patch, I think there are a couple things that I'd prefer
be done differently.
First, TCMU_DEV_BIT_ASYNC is being set by the configuration tool (i.e.
the entity who writes to configfs to establish the device.) I view out
of order (OoO) completion of commands as a TCMU implementation detail
that it doesn't need to care about. Only the kernel and the userspace
TCMU device handler need to negotiate this capability.
Second, I'd like to keep separate the idea of using random data ring
allocation from the idea of completing commands out of order. Although
certainly the second one depends on the first. (And it's not random,
it's probably better to say heap-like data ring allocation, in
comparison to the stack-like allocation tcmu currently does.) This patch
changes the circular data ring into a FIFO stack of pages. This is not
what we want for async.
Third, clearly we're going to need to decide on a granularity for the
heap allocator but I don't think it should be PAGE_SIZE. Maybe we want
the value 4096, but on some archs PAGE_SIZE is 64kB. I think 512 might
be a better initial value.
So what I'd like to see in a v2 patch is *just* changing the data area
so it acts more like a heap instead of a ringbuffer -- replace the head
and tail stuff with something that allocates, probably using a bitmap of
used and free chunks. There should even be existing in-kernel APIs to
help manage this. Once that's working, then we can talk about another
change extending the kernel-to-handler API so userspace knows it can
return commands OoO.
Thanks! -- Regards -- Andy
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
