Re: [PATCH v4 iproute2 2/7] rdma: Add dev object

[Leon Romanovsky]

On Tue, Aug 15, 2017 at 05:00:54PM +, David Laight wrote:
> From: Leon Romanovsky
> > Sent: 15 August 2017 17:47
> > On Tue, Aug 15, 2017 at 04:23:11PM +, David Laight wrote:
> > > From: Stephen Hemminger
> > > > Sent: 15 August 2017 17:12
> > > > On Tue, 15 Aug 2017 16:00:15 +0300
> > > > Leon Romanovsky  wrote:
> > > >
> > > > > +
> > > > > +static const char *dev_caps_to_str(uint32_t idx)
> > > > > +{
> > > > > + uint64_t cap = 1 << idx;
> > > > > +
> > > > > + switch (cap) {
> > > > > + case RDMA_DEV_RESIZE_MAX_WR: return "RESIZE_MAX_WR";
> > > > > + case RDMA_DEV_BAD_PKEY_CNTR: return "BAD_PKEY_CNTR";
> > > ...
> > > > > + case RDMA_DEV_RAW_SCATTER_FCS: return "RAW_SCATTER_FCS";
> > > > > + case RDMA_DEV_RDMA_NETDEV_OPA_VNIC: return 
> > > > > "RDMA_NETDEV_OPA_VNIC";
> > > > > + default: return "UNKNOWN";
> > > > > + }
> > > >
> > > > Could this be a table in future versions?
> > >
> > > Potentially you could define the constants using some pre-processor
> > > 'magic' that would create the table for you.
> > > Something like (but not compiled):
> > >
> > > #define RDMA_DEV_FLAGS(x) \
> > >   x(RESIZE_MAX_WR, 0) \
> > >   x(BAD_PKEY_CNTR, 1) \
> > > (continue for all the bits)
> > >
> > > #define RDMA_DEV_ENUM(name, bit_no) RDMA_DEV_##name = BIT(bit_no),
> > > enum {RDMA_DEV_FLAGS(RDMA_DEV_ENUM)};
> > > #undef RDMA_DEV_ENUM
> > >
> > > #define RDMA_DEV_NAMES(name, bit_no) [bit_no] = #name,
> > > static const char rdma_dev_names[] = {RDMA_DEV_FLAGS(RDMA_DEV_NAMES)};
> I missed the #undef
> > >
> >
> > David,
> >
> > How should I handle "uknown" fields without names?
>
> The function that accesses rdma_dev_names[] checks ARRAY_SIZE()
> and for a NULL pointer.

Thanks, I'll definitely try it.

>
> WRT the other comment, the spare pointers consume far less space than
> the code for your switch statement.

I don't think that space is matter for this tool.

>
>   David
>


signature.asc
Description: PGP signature

RE: [PATCH v4 iproute2 2/7] rdma: Add dev object

[David Laight]

From: Leon Romanovsky
> Sent: 15 August 2017 17:47
> On Tue, Aug 15, 2017 at 04:23:11PM +, David Laight wrote:
> > From: Stephen Hemminger
> > > Sent: 15 August 2017 17:12
> > > On Tue, 15 Aug 2017 16:00:15 +0300
> > > Leon Romanovsky  wrote:
> > >
> > > > +
> > > > +static const char *dev_caps_to_str(uint32_t idx)
> > > > +{
> > > > +   uint64_t cap = 1 << idx;
> > > > +
> > > > +   switch (cap) {
> > > > +   case RDMA_DEV_RESIZE_MAX_WR: return "RESIZE_MAX_WR";
> > > > +   case RDMA_DEV_BAD_PKEY_CNTR: return "BAD_PKEY_CNTR";
> > ...
> > > > +   case RDMA_DEV_RAW_SCATTER_FCS: return "RAW_SCATTER_FCS";
> > > > +   case RDMA_DEV_RDMA_NETDEV_OPA_VNIC: return 
> > > > "RDMA_NETDEV_OPA_VNIC";
> > > > +   default: return "UNKNOWN";
> > > > +   }
> > >
> > > Could this be a table in future versions?
> >
> > Potentially you could define the constants using some pre-processor
> > 'magic' that would create the table for you.
> > Something like (but not compiled):
> >
> > #define RDMA_DEV_FLAGS(x) \
> > x(RESIZE_MAX_WR, 0) \
> > x(BAD_PKEY_CNTR, 1) \
> > (continue for all the bits)
> >
> > #define RDMA_DEV_ENUM(name, bit_no) RDMA_DEV_##name = BIT(bit_no),
> > enum {RDMA_DEV_FLAGS(RDMA_DEV_ENUM)};
> > #undef RDMA_DEV_ENUM
> >
> > #define RDMA_DEV_NAMES(name, bit_no) [bit_no] = #name,
> > static const char rdma_dev_names[] = {RDMA_DEV_FLAGS(RDMA_DEV_NAMES)};
I missed the #undef
> >
> 
> David,
> 
> How should I handle "uknown" fields without names?

The function that accesses rdma_dev_names[] checks ARRAY_SIZE()
and for a NULL pointer.

WRT the other comment, the spare pointers consume far less space than
the code for your switch statement.

David

Re: [PATCH v4 iproute2 2/7] rdma: Add dev object

[Leon Romanovsky]

On Tue, Aug 15, 2017 at 04:23:11PM +, David Laight wrote:
> From: Stephen Hemminger
> > Sent: 15 August 2017 17:12
> > On Tue, 15 Aug 2017 16:00:15 +0300
> > Leon Romanovsky  wrote:
> >
> > > +
> > > +static const char *dev_caps_to_str(uint32_t idx)
> > > +{
> > > + uint64_t cap = 1 << idx;
> > > +
> > > + switch (cap) {
> > > + case RDMA_DEV_RESIZE_MAX_WR: return "RESIZE_MAX_WR";
> > > + case RDMA_DEV_BAD_PKEY_CNTR: return "BAD_PKEY_CNTR";
> ...
> > > + case RDMA_DEV_RAW_SCATTER_FCS: return "RAW_SCATTER_FCS";
> > > + case RDMA_DEV_RDMA_NETDEV_OPA_VNIC: return "RDMA_NETDEV_OPA_VNIC";
> > > + default: return "UNKNOWN";
> > > + }
> >
> > Could this be a table in future versions?
>
> Potentially you could define the constants using some pre-processor
> 'magic' that would create the table for you.
> Something like (but not compiled):
>
> #define RDMA_DEV_FLAGS(x) \
>   x(RESIZE_MAX_WR, 0) \
>   x(BAD_PKEY_CNTR, 1) \
> (continue for all the bits)
>
> #define RDMA_DEV_ENUM(name, bit_no) RDMA_DEV_##name = BIT(bit_no),
> enum {RDMA_DEV_FLAGS(RDMA_DEV_ENUM)};
> #undef RDMA_DEV_ENUM
>
> #define RDMA_DEV_NAMES(name, bit_no) [bit_no] = #name,
> static const char rdma_dev_names[] = {RDMA_DEV_FLAGS(RDMA_DEV_NAMES)};
>

David,

How should I handle "uknown" fields without names?

Thanks

>   David
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature

Re: [PATCH v4 iproute2 2/7] rdma: Add dev object

[Leon Romanovsky]

On Tue, Aug 15, 2017 at 09:12:05AM -0700, Stephen Hemminger wrote:
> On Tue, 15 Aug 2017 16:00:15 +0300
> Leon Romanovsky  wrote:
>
> > +
> > +static const char *dev_caps_to_str(uint32_t idx)
> > +{
> > +   uint64_t cap = 1 << idx;
> > +
> > +   switch (cap) {
> > +   case RDMA_DEV_RESIZE_MAX_WR: return "RESIZE_MAX_WR";
> > +   case RDMA_DEV_BAD_PKEY_CNTR: return "BAD_PKEY_CNTR";
> > +   case RDMA_DEV_BAD_QKEY_CNTR: return "BAD_QKEY_CNTR";
> > +   case RDMA_DEV_RAW_MULTI: return "RAW_MULTI";
> > +   case RDMA_DEV_AUTO_PATH_MIG: return "AUTO_PATH_MIG";
> > +   case RDMA_DEV_CHANGE_PHY_PORT: return "CHANGE_PHY_POR";
> > +   case RDMA_DEV_UD_AV_PORT_ENFORCE: return "UD_AV_PORT_ENFORCE";
> > +   case RDMA_DEV_CURR_QP_STATE_MOD: return "CURR_QP_STATE_MOD";
> > +   case RDMA_DEV_SHUTDOWN_PORT: return "SHUTDOWN_PORT";
> > +   case RDMA_DEV_INIT_TYPE: return "INIT_TYPE";
> > +   case RDMA_DEV_PORT_ACTIVE_EVENT: return "PORT_ACTIVE_EVENT";
> > +   case RDMA_DEV_SYS_IMAGE_GUID: return "SYS_IMAGE_GUID";
> > +   case RDMA_DEV_RC_RNR_NAK_GEN: return "RC_RNR_NAK_GEN";
> > +   case RDMA_DEV_SRQ_RESIZE: return "SRQ_RESIZE";
> > +   case RDMA_DEV_N_NOTIFY_CQ: return "N_NOTIFY_CQ";
> > +   case RDMA_DEV_LOCAL_DMA_LKEY: return "LOCAL_DMA_LKEY";
> > +   case RDMA_DEV_MEM_WINDOW: return "MEM_WINDOW";
> > +   case RDMA_DEV_UD_IP_CSUM: return "UD_IP_CSUM";
> > +   case RDMA_DEV_UD_TSO: return "UD_TSO";
> > +   case RDMA_DEV_XRC: return "XRC";
> > +   case RDMA_DEV_MEM_MGT_EXTENSIONS: return "MEM_MGT_EXTENSIONS";
> > +   case RDMA_DEV_BLOCK_MULTICAST_LOOPBACK:
> > +   return "BLOCK_MULTICAST_LOOPBACK";
> > +   case RDMA_DEV_MEM_WINDOW_TYPE_2A: return "MEM_WINDOW_TYPE_2A";
> > +   case RDMA_DEV_MEM_WINDOW_TYPE_2B: return "MEM_WINDOW_TYPE_2B";
> > +   case RDMA_DEV_RC_IP_CSUM: return "RC_IP_CSUM";
> > +   case RDMA_DEV_RAW_IP_CSUM: return "RAW_IP_CSUM";
> > +   case RDMA_DEV_CROSS_CHANNEL: return "CROSS_CHANNEL";
> > +   case RDMA_DEV_MANAGED_FLOW_STEERING: return "MANAGED_FLOW_STEERING";
> > +   case RDMA_DEV_SIGNATURE_HANDOVER: return "SIGNATURE_HANDOVER";
> > +   case RDMA_DEV_ON_DEMAND_PAGING: return "ON_DEMAND_PAGING";
> > +   case RDMA_DEV_SG_GAPS_REG: return "SG_GAPS_REG";
> > +   case RDMA_DEV_VIRTUAL_FUNCTION: return "VIRTUAL_FUNCTION";
> > +   case RDMA_DEV_RAW_SCATTER_FCS: return "RAW_SCATTER_FCS";
> > +   case RDMA_DEV_RDMA_NETDEV_OPA_VNIC: return "RDMA_NETDEV_OPA_VNIC";
> > +   default: return "UNKNOWN";
> > +   }
>
> Could this be a table in future versions?

The device capability is 64 bit field -> the table will have 64 entry,
while most of them should be marked as "UNKNOWN". Because we have holes
[1] in the fields (for example bit 9), I can't use simple
"if (idx > max_value) return "uknown"" logic.

So the answer is yes, I can do it as a table, but it won't look better than
it is now :)

By writing "future versions", are you asking me to resubmit? or followup patch
will be possible solution too?

Thanks

[1] http://marc.info/?l=linux-rdma=150278850520856=2

> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature

RE: [PATCH v4 iproute2 2/7] rdma: Add dev object

[David Laight]

From: Stephen Hemminger
> Sent: 15 August 2017 17:12
> On Tue, 15 Aug 2017 16:00:15 +0300
> Leon Romanovsky  wrote:
> 
> > +
> > +static const char *dev_caps_to_str(uint32_t idx)
> > +{
> > +   uint64_t cap = 1 << idx;
> > +
> > +   switch (cap) {
> > +   case RDMA_DEV_RESIZE_MAX_WR: return "RESIZE_MAX_WR";
> > +   case RDMA_DEV_BAD_PKEY_CNTR: return "BAD_PKEY_CNTR";
...
> > +   case RDMA_DEV_RAW_SCATTER_FCS: return "RAW_SCATTER_FCS";
> > +   case RDMA_DEV_RDMA_NETDEV_OPA_VNIC: return "RDMA_NETDEV_OPA_VNIC";
> > +   default: return "UNKNOWN";
> > +   }
> 
> Could this be a table in future versions?

Potentially you could define the constants using some pre-processor
'magic' that would create the table for you.
Something like (but not compiled):

#define RDMA_DEV_FLAGS(x) \
x(RESIZE_MAX_WR, 0) \
x(BAD_PKEY_CNTR, 1) \
(continue for all the bits)

#define RDMA_DEV_ENUM(name, bit_no) RDMA_DEV_##name = BIT(bit_no),
enum {RDMA_DEV_FLAGS(RDMA_DEV_ENUM)};
#undef RDMA_DEV_ENUM

#define RDMA_DEV_NAMES(name, bit_no) [bit_no] = #name,
static const char rdma_dev_names[] = {RDMA_DEV_FLAGS(RDMA_DEV_NAMES)};

David

Re: [PATCH v4 iproute2 2/7] rdma: Add dev object

[Stephen Hemminger]

On Tue, 15 Aug 2017 16:00:15 +0300
Leon Romanovsky  wrote:

> +
> +static const char *dev_caps_to_str(uint32_t idx)
> +{
> + uint64_t cap = 1 << idx;
> +
> + switch (cap) {
> + case RDMA_DEV_RESIZE_MAX_WR: return "RESIZE_MAX_WR";
> + case RDMA_DEV_BAD_PKEY_CNTR: return "BAD_PKEY_CNTR";
> + case RDMA_DEV_BAD_QKEY_CNTR: return "BAD_QKEY_CNTR";
> + case RDMA_DEV_RAW_MULTI: return "RAW_MULTI";
> + case RDMA_DEV_AUTO_PATH_MIG: return "AUTO_PATH_MIG";
> + case RDMA_DEV_CHANGE_PHY_PORT: return "CHANGE_PHY_POR";
> + case RDMA_DEV_UD_AV_PORT_ENFORCE: return "UD_AV_PORT_ENFORCE";
> + case RDMA_DEV_CURR_QP_STATE_MOD: return "CURR_QP_STATE_MOD";
> + case RDMA_DEV_SHUTDOWN_PORT: return "SHUTDOWN_PORT";
> + case RDMA_DEV_INIT_TYPE: return "INIT_TYPE";
> + case RDMA_DEV_PORT_ACTIVE_EVENT: return "PORT_ACTIVE_EVENT";
> + case RDMA_DEV_SYS_IMAGE_GUID: return "SYS_IMAGE_GUID";
> + case RDMA_DEV_RC_RNR_NAK_GEN: return "RC_RNR_NAK_GEN";
> + case RDMA_DEV_SRQ_RESIZE: return "SRQ_RESIZE";
> + case RDMA_DEV_N_NOTIFY_CQ: return "N_NOTIFY_CQ";
> + case RDMA_DEV_LOCAL_DMA_LKEY: return "LOCAL_DMA_LKEY";
> + case RDMA_DEV_MEM_WINDOW: return "MEM_WINDOW";
> + case RDMA_DEV_UD_IP_CSUM: return "UD_IP_CSUM";
> + case RDMA_DEV_UD_TSO: return "UD_TSO";
> + case RDMA_DEV_XRC: return "XRC";
> + case RDMA_DEV_MEM_MGT_EXTENSIONS: return "MEM_MGT_EXTENSIONS";
> + case RDMA_DEV_BLOCK_MULTICAST_LOOPBACK:
> + return "BLOCK_MULTICAST_LOOPBACK";
> + case RDMA_DEV_MEM_WINDOW_TYPE_2A: return "MEM_WINDOW_TYPE_2A";
> + case RDMA_DEV_MEM_WINDOW_TYPE_2B: return "MEM_WINDOW_TYPE_2B";
> + case RDMA_DEV_RC_IP_CSUM: return "RC_IP_CSUM";
> + case RDMA_DEV_RAW_IP_CSUM: return "RAW_IP_CSUM";
> + case RDMA_DEV_CROSS_CHANNEL: return "CROSS_CHANNEL";
> + case RDMA_DEV_MANAGED_FLOW_STEERING: return "MANAGED_FLOW_STEERING";
> + case RDMA_DEV_SIGNATURE_HANDOVER: return "SIGNATURE_HANDOVER";
> + case RDMA_DEV_ON_DEMAND_PAGING: return "ON_DEMAND_PAGING";
> + case RDMA_DEV_SG_GAPS_REG: return "SG_GAPS_REG";
> + case RDMA_DEV_VIRTUAL_FUNCTION: return "VIRTUAL_FUNCTION";
> + case RDMA_DEV_RAW_SCATTER_FCS: return "RAW_SCATTER_FCS";
> + case RDMA_DEV_RDMA_NETDEV_OPA_VNIC: return "RDMA_NETDEV_OPA_VNIC";
> + default: return "UNKNOWN";
> + }

Could this be a table in future versions?