Re: [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
>> http://www.t10.org/lists/op-num.txt. Entries in that file look like this:
>>
>> OP DTLPWROMAEBKVF Description
>> -- --
>> 00 MM TEST UNIT READY
>> 01 M REWIND
>> 01 Z V 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/
Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
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 MM TEST UNIT READY
> 01 M REWIND
> 01 Z V 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.
> > 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 guess this ties to the other
thread on why it's desirable to add all listed commands. 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?
> > 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 //.
> >> 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.
> >> #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)?
Thanks.
--
tejun
--
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/
Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
Il 24/01/2013 23:58, Tejun Heo ha scritto:
>
>> +#define sgio_bitmap_set(cmd, mask, rw) \
>> +if ((mask) != 0) __set_bit((cmd), filter->rw##_ok)
>> +
>> +#define D (1u << TYPE_DISK) /* Direct Access Block Device (SBC-3)
>> */
>> +#define T (1u << TYPE_TAPE) /* Sequential Access Device (SSC-3) */
>> +#define L (1u << TYPE_PRINTER)/* Printer Device (SSC) */
>> +#define P (1u << TYPE_PROCESSOR) /* Processor Device (SPC-2) */
>> +#define W (1u << TYPE_WORM) /* Write Once Block Device (SBC) */
>> +#define R (1u << TYPE_ROM)/* C/DVD Device (MMC-6) */
>> +#define S (1u << TYPE_SCANNER)/* Scanner device (obsolete) */
>> +#define O (1u << TYPE_MOD)/* Optical Memory Block Device (SBC)
>> */
>> +#define M (1u << TYPE_MEDIUM_CHANGER) /* Media Changer Device (SMC-3) */
>> +#define C (1u << TYPE_COMM) /* Communication devices (obsolete) */
>> +#define A (1u << TYPE_RAID) /* Storage Array Device (SCC-2) */
>> +#define E (1u << TYPE_ENCLOSURE) /* SCSI Enclosure Services device
>> (SES-2) */
>> +#define B (1u << TYPE_RBC)/* Simplified Direct-Access (Reduced
>> Block) device (RBC) */
>> +#define K (1u << 0x0f)/* Optical Card Reader/Writer device
>> (OCRW) */
>> +#define V (1u << 0x10)/* Automation/Device Interface device
>> (ADC-2) */
>
> Can we please add TYPE_* constants for these two too?
Ok.
>> +#define F (1u << TYPE_OSD)/* Object-based Storage Device
>> (OSD-2) */
>> +
>> +/* control, universal except possibly RBC, read */
>> +
>> +sgio_bitmap_set(0x00, -1 , read); // TEST
>> UNIT READY
>
> Hmmm... why are we not using symbolic constants for commands?
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 MM TEST UNIT READY
01 M REWIND
01 Z V REZERO UNIT
which explains a bit the formatting.
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.
> 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.
> 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.
// 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.
>>> One other thing is I would much prefer if the table was made static
>>> const first. As we only allow compile-time defined tables, there's no
>>> point in dynamically initializing these and the above can be static
>>> initializers.
>>
>> 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.
>> #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.
> Also, I
> still think device -> command organization would be better than
> commmand -> device.
This patch ties to keep the list as similar as possible to the old one,
to ease review. There is usually a 1:1 matching between "old" and "new"
lines, and this limits the freedom I have in organizing the list.
In the next patches I
Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
Il 24/01/2013 23:58, Tejun Heo ha scritto:
+#define sgio_bitmap_set(cmd, mask, rw) \
+if ((mask) != 0) __set_bit((cmd), filter-rw##_ok)
+
+#define D (1u TYPE_DISK) /* Direct Access Block Device (SBC-3)
*/
+#define T (1u TYPE_TAPE) /* Sequential Access Device (SSC-3) */
+#define L (1u TYPE_PRINTER)/* Printer Device (SSC) */
+#define P (1u TYPE_PROCESSOR) /* Processor Device (SPC-2) */
+#define W (1u TYPE_WORM) /* Write Once Block Device (SBC) */
+#define R (1u TYPE_ROM)/* C/DVD Device (MMC-6) */
+#define S (1u TYPE_SCANNER)/* Scanner device (obsolete) */
+#define O (1u TYPE_MOD)/* Optical Memory Block Device (SBC)
*/
+#define M (1u TYPE_MEDIUM_CHANGER) /* Media Changer Device (SMC-3) */
+#define C (1u TYPE_COMM) /* Communication devices (obsolete) */
+#define A (1u TYPE_RAID) /* Storage Array Device (SCC-2) */
+#define E (1u TYPE_ENCLOSURE) /* SCSI Enclosure Services device
(SES-2) */
+#define B (1u TYPE_RBC)/* Simplified Direct-Access (Reduced
Block) device (RBC) */
+#define K (1u 0x0f)/* Optical Card Reader/Writer device
(OCRW) */
+#define V (1u 0x10)/* Automation/Device Interface device
(ADC-2) */
Can we please add TYPE_* constants for these two too?
Ok.
+#define F (1u TYPE_OSD)/* Object-based Storage Device
(OSD-2) */
+
+/* control, universal except possibly RBC, read */
+
+sgio_bitmap_set(0x00, -1 , read); // TEST
UNIT READY
Hmmm... why are we not using symbolic constants for commands?
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 MM TEST UNIT READY
01 M REWIND
01 Z V REZERO UNIT
which explains a bit the formatting.
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.
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.
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.
// 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.
One other thing is I would much prefer if the table was made static
const first. As we only allow compile-time defined tables, there's no
point in dynamically initializing these and the above can be static
initializers.
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.
#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.
Also, I
still think device - command organization would be better than
commmand - device.
This patch ties to keep the list as similar as possible to the old one,
to ease review. There is usually a 1:1 matching between old and new
lines, and this limits the freedom I have in organizing the list.
In the next patches I gradually move towards a more structured
organization, usually something like standard-command (one standard may
correspond to more than one device type, but all of them are
Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
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 MM TEST UNIT READY
01 M REWIND
01 Z V 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.
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 guess this ties to the other
thread on why it's desirable to add all listed commands. 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?
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 //.
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.
#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)?
Thanks.
--
tejun
--
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/
Re: [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
http://www.t10.org/lists/op-num.txt. Entries in that file look like this:
OP DTLPWROMAEBKVF Description
-- --
00 MM TEST UNIT READY
01 M REWIND
01 Z V 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/
Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
On Thu, Jan 24, 2013 at 02:49:21PM -0800, Tejun Heo wrote:
> On Thu, Jan 24, 2013 at 02:42:03PM -0800, Tejun Heo wrote:
> > One other thing is I would much prefer if the table was made static
> > const first. As we only allow compile-time defined tables, there's no
> > point in dynamically initializing these and the above can be static
> > initializers.
>
> 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,
>
> #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. Also, I
still think device -> command organization would be better than
commmand -> device.
Thanks.
--
tejun
--
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/
Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
On Thu, Jan 24, 2013 at 02:42:03PM -0800, Tejun Heo wrote:
> One other thing is I would much prefer if the table was made static
> const first. As we only allow compile-time defined tables, there's no
> point in dynamically initializing these and the above can be static
> initializers.
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,
#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) |
...
};
Thanks.
--
tejun
--
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/
Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
On Thu, Jan 24, 2013 at 04:00:38PM +0100, Paolo Bonzini wrote: > +#define sgio_bitmap_set(cmd, mask, rw) \ > + if ((mask) != 0) __set_bit((cmd), filter->rw##_ok) > + > +#define D (1u << TYPE_DISK) /* Direct Access Block Device (SBC-3) > */ > +#define T (1u << TYPE_TAPE) /* Sequential Access Device (SSC-3) */ > +#define L (1u << TYPE_PRINTER)/* Printer Device (SSC) */ > +#define P (1u << TYPE_PROCESSOR) /* Processor Device (SPC-2) */ > +#define W (1u << TYPE_WORM) /* Write Once Block Device (SBC) */ > +#define R (1u << TYPE_ROM)/* C/DVD Device (MMC-6) */ > +#define S (1u << TYPE_SCANNER)/* Scanner device (obsolete) */ > +#define O (1u << TYPE_MOD)/* Optical Memory Block Device (SBC) */ > +#define M (1u << TYPE_MEDIUM_CHANGER) /* Media Changer Device (SMC-3) */ > +#define C (1u << TYPE_COMM) /* Communication devices (obsolete) */ > +#define A (1u << TYPE_RAID) /* Storage Array Device (SCC-2) */ > +#define E (1u << TYPE_ENCLOSURE) /* SCSI Enclosure Services device > (SES-2) */ > +#define B (1u << TYPE_RBC)/* Simplified Direct-Access (Reduced > Block) device (RBC) */ > +#define K (1u << 0x0f)/* Optical Card Reader/Writer device > (OCRW) */ > +#define V (1u << 0x10)/* Automation/Device Interface device > (ADC-2) */ Can we please add TYPE_* constants for these two too? > +#define F (1u << TYPE_OSD)/* Object-based Storage Device (OSD-2) > */ > + > + /* control, universal except possibly RBC, read */ > + > + sgio_bitmap_set(0x00, -1 , read); // TEST > UNIT READY Hmmm... why are we not using symbolic constants for commands? If it's because of horizontal real estate, we can abbreviate sgio_bitmap_set(), no? 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 //? One other thing is I would much prefer if the table was made static const first. As we only allow compile-time defined tables, there's no point in dynamically initializing these and the above can be static initializers. Thanks. -- tejun -- 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/
Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
On Thu, Jan 24, 2013 at 04:00:38PM +0100, Paolo Bonzini wrote: +#define sgio_bitmap_set(cmd, mask, rw) \ + if ((mask) != 0) __set_bit((cmd), filter-rw##_ok) + +#define D (1u TYPE_DISK) /* Direct Access Block Device (SBC-3) */ +#define T (1u TYPE_TAPE) /* Sequential Access Device (SSC-3) */ +#define L (1u TYPE_PRINTER)/* Printer Device (SSC) */ +#define P (1u TYPE_PROCESSOR) /* Processor Device (SPC-2) */ +#define W (1u TYPE_WORM) /* Write Once Block Device (SBC) */ +#define R (1u TYPE_ROM)/* C/DVD Device (MMC-6) */ +#define S (1u TYPE_SCANNER)/* Scanner device (obsolete) */ +#define O (1u TYPE_MOD)/* Optical Memory Block Device (SBC) */ +#define M (1u TYPE_MEDIUM_CHANGER) /* Media Changer Device (SMC-3) */ +#define C (1u TYPE_COMM) /* Communication devices (obsolete) */ +#define A (1u TYPE_RAID) /* Storage Array Device (SCC-2) */ +#define E (1u TYPE_ENCLOSURE) /* SCSI Enclosure Services device (SES-2) */ +#define B (1u TYPE_RBC)/* Simplified Direct-Access (Reduced Block) device (RBC) */ +#define K (1u 0x0f)/* Optical Card Reader/Writer device (OCRW) */ +#define V (1u 0x10)/* Automation/Device Interface device (ADC-2) */ Can we please add TYPE_* constants for these two too? +#define F (1u TYPE_OSD)/* Object-based Storage Device (OSD-2) */ + + /* control, universal except possibly RBC, read */ + + sgio_bitmap_set(0x00, -1 , read); // TEST UNIT READY Hmmm... why are we not using symbolic constants for commands? If it's because of horizontal real estate, we can abbreviate sgio_bitmap_set(), no? 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 //? One other thing is I would much prefer if the table was made static const first. As we only allow compile-time defined tables, there's no point in dynamically initializing these and the above can be static initializers. Thanks. -- tejun -- 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/
Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
On Thu, Jan 24, 2013 at 02:42:03PM -0800, Tejun Heo wrote:
One other thing is I would much prefer if the table was made static
const first. As we only allow compile-time defined tables, there's no
point in dynamically initializing these and the above can be static
initializers.
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,
#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) |
...
};
Thanks.
--
tejun
--
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/
Re: [PATCH 02/13] sg_io: reorganize list of allowed commands
On Thu, Jan 24, 2013 at 02:49:21PM -0800, Tejun Heo wrote:
On Thu, Jan 24, 2013 at 02:42:03PM -0800, Tejun Heo wrote:
One other thing is I would much prefer if the table was made static
const first. As we only allow compile-time defined tables, there's no
point in dynamically initializing these and the above can be static
initializers.
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,
#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. Also, I
still think device - command organization would be better than
commmand - device.
Thanks.
--
tejun
--
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/
