Andy, >> On 01 Dec 2015, at 01:24, Andy Grover <agrover@xxxxxxxxxx> wrote: >> >> 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. Ok! >> 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. This might be a problem as I use sg lists to remember which data chunks need to be free'd (I use dma_address for that). > 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. The idea was to keep the original data allocator intact at least in the first version of the patch. This can of course be changed to always using the new data allocator and a capability returned by the kernel that it can do ooo completion. > 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. It is not a FIFO stack. data_pages_used is actually a free-list so data pages can be allocated and freed in random order. > 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. I can change that but this will complicate things as sg list is PAGE_SIZE'd. Also, page-size based allocation is convenient if we want to have a zero-copy TCMU (this is actually a long-term goal, or 2nd stage feature that we would like to have). > 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. I was not able to find a ready-to-use in-kernel arena allocator (that can allocate memory within the specified continuous memory chunk) so I ended up with a free-list implementation. I thought that full-featured bitmap or slab allocator solely for TCMU is an overkill. Max-- 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
