[lkml]   [2013]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 02/13] sg_io: reorganize list of allowed commands
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
>> Entries in that file look like this:
>> -- -------------- ----------------------------------------------------
>> 01 M REWIND
>> 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 |
>>>> ...
>>>> };
>>> 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.


 \ /
  Last update: 2013-01-25 19:01    [W:0.051 / U:4.936 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site