Il 25/01/2013 18:13, Tejun Heo ha scritto:
> Hello, Paolo.
>
> On Fri, Jan 25, 2013 at 11:01:38AM +0100, Paolo Bonzini wrote:
>> First, because the table is based on
>> http://www.t10.org/lists/op-num.txt. Entries in that file look like this:
>>
>> OP DTLPWROMAEBKVF Description
>> -- -------------- ----------------------------------------------------
>> 00 MMMMMMMMMMMMMM TEST UNIT READY
>> 01 M REWIND
>> 01 Z V ZZZZ REZERO UNIT
>>
>> which explains a bit the formatting.
>
> Ah, okay, if it's something already established, please go ahead.
>
>> Second, because many symbolic constants do not exist in
>> include/scsi/scsi.h or include/linux/uapi/cdrom.h, and I don't think it
>> would make sense to add them for a one-off use, especially for obsolete
>> commands or device types.
>
> It's kinda nice to be able to search for the constants for usages in
> kernel. It's not complete but does help from time to time.
Yeah, that's true. On the other hand, all the constants that are
missing are really just for userspace tools in general.
>>> If it's because of horizontal real estate, we can abbreviate
>>> sgio_bitmap_set(), no?
>>
>> No, it's not that. In fact using the symbolic constants would save a
>> few characters (for the 0xNN and the comment start). I really prefer to
>> keep the opcode visible so that you can easily match the code to op-num.txt.
>
> How many constants need to be added?
I'd guesstimate 40-50.
> If you're
> just gonna add several, there's no point in not using the constants,
> right? Most are already there. If you want opcodes visible, you can
> make them the comments, right?
Yes, like "/* 0x00 */ CONSTANT, MASK". I still have a slight preference
for the opcodes because if the constant ends up wrong, the
head-scratching would be higher than if the opcode is wrong (the opcode
is what you see in the dumps).
>>> Also, wouldn't it be better to have ALL
>>> instead of -1? Also, the custom formatting is nice but can we at
>>> least not use //?
>>
>> ALL instead of -1 is a good idea, or I can just spell it out if it's
>> okay for you.
>
> Yeah, both sound fine to me.
>
>> // is nicer in my opinion (for this case where we're throwing away all
>> the rules anyway) because it avoids the misaligned */ but I can change
>> it of course.
>
> Let's please not do //.
Ok, // comments replaced with C comments.
>>>> On the similar line of thoughts, wouldn't it be better to have the
>>>> table organized by the device type first? It would be much easier to
>>>> comprehend which commands are allowed for each device type that way
>>>> and FWIW it would be more cacheline friendly. e.g. something like,
>>
>> It is actually more cacheline friendly like this. The vast majority of
>> commands will be READ and WRITE, which are 0x?8 and 0x?A. So in
>> practice one cacheline will be shared by all device types, maybe two if
>> you use READ/WRITE(12) for some devices and READ/WRITE(16) for others.
>
> The cacheline thing was me being confused about how the tables are
> used. The tables are per-device after initialized so it should be
> fine.
No, they are not, but it is fine anyway. :) You'll really use very
little of this table (and of the old one as well) in the hot paths.
>>>> #define M(opcode) (1 << opcode)
>>>>
>>>> #define COMMON \
>>>> M(READ_6) | M(WRITE_6) | ....
>>>>
>>>> static const whatever_type blk_cmd_filter_disk = {
>>>> COMMON |
>>>> M(CMD_SPECIFIC_TO_THIS_TYPE0) |
>>>> M(CMD_SPECIFIC_TO_THIS_TYPE2) |
>>>> ...
>>>> };
>>>
>>> Oops, there are way more bits than in the longest integer, so you
>>> can't statically initialize them in pretty way (maybe it's possible
>>> but I can't think of anything pretty). We can still initialize the
>>> table once during boot and throw away the init code, I guess.
>>
>> Yeah, that's what happens because GCC will inline
>> blk_set_cmd_filter_defaults into its sole caller which is __init.
>> There's no difference from before this patch, but I can add an explicit
>> __init to blk_set_cmd_filter_defaults too.
>
> Maybe I'm misreading the code but we're still initializing table per
> queue, right? Can't we just have per-type tables which are populated
> during boot (or on-demand)?
No, the queue just stores the device type in an unsigned char. The
device type is then used to pick a bit in each word. I think you are
confusing with an earlier version you saw on Red Hat mailing lists, but
you NACKed it because you didn't like the lazy allocation.
Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
