RE: [PATCH 12/13] staging/rdma/hfi1: Read EFI variable for device description

[Luick, Dean]

> -Original Message-
> From: gre...@linuxfoundation.org [mailto:gre...@linuxfoundation.org]
> Sent: Wednesday, November 11, 2015 11:24 AM

> > If you move the variables to the top and have the early return as you 
> > suggest,
> then in some CONFIG cases, there will be all those automatic variables 
> declared
> but they are never used - the compiler has short-circuited the rest of the
> function.  Will not the compiler complain about unused variables in those 
> cases?
> That is the situation I was trying to avoid.
> 
> Try it and see (hint, I don't think so...)

I tested this - you are correct, no compiler warnings.  Looks like my fears 
were premature.  The failure logic will be inverted per Dan's comments.


Dean
--
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

RE: [PATCH 12/13] staging/rdma/hfi1: Read EFI variable for device description

[Luick, Dean]

> -Original Message-
> From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
> ow...@vger.kernel.org] On Behalf Of Dan Carpenter
> Sent: Wednesday, November 11, 2015 2:45 AM
> To: John, Jubin 
> Cc: gre...@linuxfoundation.org; de...@driverdev.osuosl.org; linux-
> r...@vger.kernel.org; dledf...@redhat.com
> Subject: Re: [PATCH 12/13] staging/rdma/hfi1: Read EFI variable for device
> description
> 
> On Wed, Nov 11, 2015 at 02:33:32AM -0500, Jubin John wrote:
> > +static int read_efi_var(const char *name, unsigned long *size,
> > +   void **return_data)
> > +{
> > +   int ret;
> > +
> > +   /* set failure return values */
> > +   *size = 0;
> > +   *return_data = NULL;
> > +
> > +   /*
> > +* Use EFI run-time support to obtain an EFI variable.  Support may
> > +* be compiled out, so declare all variables inside.
> > +*/
> > +   if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> 
> 
> Flip this around:
> 
>   if (!efi_enabled(EFI_RUNTIME_SERVICES))
>   return -ENOSYS;

The style here is very deliberate.

The issue is how efi_enabled() is defined via CONFIG options.  The function can 
be turned into a 0 if certain CONFIG variables are not set.  The code is 
structured to make all of the dependent variables disappear if efi_enabled() 
becomes 0.  If the code is shifted as you suggest, we will get builds from the 
automatic builders that try all combinations with unused variables.  This was 
done to avoid that.  

The question: Which is preferred?


> > +   efi_status_t status;
> > +   efi_char16_t *uni_name;
> > +   efi_guid_t guid;
> > +   unsigned long temp_size;
> > +   void *temp_buffer;
> > +   void *data;
> > +   int i;
> > +
> > +   uni_name = kzalloc(sizeof(efi_char16_t) * (strlen(name) + 1),
> > +  GFP_KERNEL);
> > +   temp_buffer = kzalloc(EFI_DATA_SIZE, GFP_KERNEL);
> > +   data = NULL;
> 
> No need.
> 
> > +
> > +   if (!uni_name || !temp_buffer) {
> > +   ret = -ENOMEM;
> > +   goto fail;
> > +   }
> > +
> > +   /* input: the size of the buffer */
> > +   temp_size = EFI_DATA_SIZE;
> > +
> > +   /* convert ASCII to unicode - it is a 1:1 mapping */
> > +   for (i = 0; name[i]; i++)
> > +   uni_name[i] = name[i];
> > +
> > +   /* need a variable for our GUID */
> > +   guid = HFI1_EFIVAR_GUID;
> > +
> > +   /* call into EFI runtime services */
> > +   status = efi.get_variable(
> > +   uni_name,
> > +   ,
> > +   NULL,
> > +   _size,
> > +   temp_buffer);
> > +
> > +   /*
> > +* It would be nice to call efi_status_to_err() here, but that
> > +* is in the EFIVAR_FS code and may not be compiled in.
> > +* However, even that is insufficient since it does not cover
> > +* EFI_BUFFER_TOO_SMALL which could be an important
> return.
> > +* For now, just split out succces or not found.
> > +*/
> > +   ret = status == EFI_SUCCESS   ? 0 :
> > + status == EFI_NOT_FOUND ? -ENOENT :
> > +   -EINVAL;
> > +
> > +   if (!ret) {
> > +   /*
> > +* We have successfully read the EFI variable into our
> > +* temporary buffer.  Now allocate a correctly sized
> > +* buffer.
> > +*/
> > +   data = kmalloc(temp_size, GFP_KERNEL);
> > +   if (data) {
> > +   memcpy(data, temp_buffer, temp_size);
> > +   *size = temp_size;
> > +   *return_data = data;
> > +   } else {
> > +   ret = -ENOMEM;
> > +   }
> > +   }
> 
> People often change the last two conditions in the function from
> error handling to success handling.  I have ranted about it before many
> times so I should just paste a previous rant instead of commenting here.
> :P
> 
> http://www.spinics.net/lists/arm-kernel/msg457849.html
> 
> Success handling makes this look more complicated than it really is.
> This code is just a string of commands in a row with error handling.  No
> need for if statements or indenting.  Here is how it looks when it's
> pulled in one indent level and changed from success handling to error
> handling.
> 
>   ret = status == EFI_SUCCESS   ? 0 :
> status == EFI_NOT_FOUND ? -ENOENT : -EINVAL;
>   if (ret)
>   goto free;
> 
>   data = kmemdup(data, temp_size, GFP_KERNEL);
>   if (!data) {
>   ret = -ENOMEM;
>   goto free;
>   }
> 
>   *size = 

RE: [PATCH 12/13] staging/rdma/hfi1: Read EFI variable for device description

[Luick, Dean]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Wednesday, November 11, 2015 8:39 AM
> To: Luick, Dean <dean.lu...@intel.com>
> Cc: John, Jubin <jubin.j...@intel.com>; de...@driverdev.osuosl.org;
> gre...@linuxfoundation.org; dledf...@redhat.com; linux-
> r...@vger.kernel.org
> Subject: Re: [PATCH 12/13] staging/rdma/hfi1: Read EFI variable for device
> description
> 
> > > > +   if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> > >
> > >
> > > Flip this around:
> > >
> > >   if (!efi_enabled(EFI_RUNTIME_SERVICES))
> > >   return -ENOSYS;
> >
> > The style here is very deliberate.
> >
> > The issue is how efi_enabled() is defined via CONFIG options.
> >  The function can be turned into a 0 if certain CONFIG variables are
> > not set.  The code is structured to make all of the dependent
> > variables disappear if efi_enabled() becomes 0.
> 
> This all understand.
> 
> >  If the code is shifted as you suggest, we will get builds from the
> > automatic builders that try all combinations with unused variables.
> >  This was done to avoid that.
> 
> I'm not sure I understand.  You are doing this to try tricking the
> autobuilders into not testind certain configs?  What?

Certainly not.  I did not explain this well.

> I don't
> understand what you mean by unused variables.  There shouldn't be any
> unused variable warnings.  If you are getting unused variable warnings
> can you post one so that I can take a look?

If you move the variables to the top and have the early return as you suggest, 
then in some CONFIG cases, there will be all those automatic variables declared 
but they are never used - the compiler has short-circuited the rest of the 
function.  Will not the compiler complain about unused variables in those 
cases?  That is the situation I was trying to avoid.


Dean

--
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

RE: [RFC/PATCH v3] IPoIB: Leave space in skb linear buffer for IP headers

[Luick, Dean]

 From: Roland Dreier rol...@purestorage.com
 + if (wc-byte_len  IPOIB_UD_HEAD_SIZE) {
 + page = priv-rx_ring[wr_id].page;
 + priv-rx_ring[wr_id].page = NULL;
 + } else {
 + page = NULL;
 + }
 +
   /*
* If we can't allocate a new RX buffer, dump
* this packet and reuse the old buffer.
*/
   if (unlikely(!ipoib_alloc_rx_skb(dev, wr_id))) {
   ++dev-stats.rx_dropped;
 + priv-rx_ring[wr_id].page = page;
   goto repost;
   }


Can you go through the else of the first if (page is NULL), then enter the 
second if? If so, isn't the page lost?


Dean
--
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

RE: Qperf and APM

[Luick, Dean]

Using QLogic dual port HCAs, this works for me when I disable port 1 on the 
client, server, or both while the test is running:

# qperf server ipaddr -ap 2 -t 30 rc_bw
rc_bw:
warning: -ap set but not used in test rc_bw
bw  =  272 MB/sec

The warning can be ignored.  Using -ap is equivalent to using -rap and -lap 
with the same port.

The failed to receive results is qperf saying that once the test finished, 
the server did not contact the client.


Dean

 -Original Message-
 From: Suresh Shelvapille [mailto:s...@baymicrosystems.com]
 Sent: Wednesday, March 06, 2013 4:24 PM
 To: Luick, Dean; linux-rdma@vger.kernel.org
 Subject: RE: Qperf and APM
 
 I can see the packets going on the alternate path when the first link fails, 
 but
 at the end of the test, the result is a
 failure, so I am not sure!
 I tried this with both '-ap' and -lap -rap options. I am assuming they are
 equivalent.
 
 --
 [root]# qperf server-ipaddr -ap 2 -t 30 rc_bw
 rc_bw:
 warning: -ap set but not used in test rc_bw
 failed to receive results: timed out
 
 
 
 -
 [root]# qperf server-ipaddr -lap 2 -rap 2 -t 30 rc_bw
 rc_bw:
 warning: -lap set but not used in test rc_bw
 warning: -rap set but not used in test rc_bw
 failed to receive results: timed out
 
 thanks,
 Suri
 
  -Original Message-
  From: Luick, Dean [mailto:dean.lu...@intel.com]
  Sent: Wednesday, March 06, 2013 3:57 PM
  To: Suresh Shelvapille; linux-rdma@vger.kernel.org
  Subject: RE: Qperf and APM
 
  Have you tried ignoring the warning and checking of the alternate port
 works?  The qperf code looks
  to be using the alternate port, but incorrectly fails to mark the argument 
  as
 used.
 
  Dean
 
   -Original Message-
   From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
   ow...@vger.kernel.org] On Behalf Of Suresh Shelvapille
   Sent: Wednesday, March 06, 2013 1:00 PM
   To: Luick, Dean; linux-rdma@vger.kernel.org
   Subject: RE: Qperf and APM
  
   This is the error I see on the client(the same warning is seen with -lap 
   and
 -
   rap options):
  
   --
   qperf server-ip-addr -ap 2 rc_bw
   rc_bw:
   warning: -ap set but not used in test rc_bw
   bw  =  3.39 GB/sec
  
-Original Message-
From: Luick, Dean [mailto:dean.lu...@intel.com]
Sent: Wednesday, March 06, 2013 1:52 PM
To: Suresh Shelvapille; linux-rdma@vger.kernel.org
Cc: Hefty, Sean
Subject: RE: Qperf and APM
   
Hi,
   
Can you be more specific?  What are you running and how is it failing?
   
Thanks,
Dean
   
 -Original Message-
 From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
 ow...@vger.kernel.org] On Behalf Of Suresh Shelvapille
 Sent: Wednesday, March 06, 2013 9:40 AM
 To: linux-rdma@vger.kernel.org
 Cc: Hefty, Sean
 Subject: Qperf and APM

 Folks:
 The qperf utility has -ap, -lap, -rap options in support of APM.
   Unfortunately
 they don't seem to work. Is there a way
 to make this work, or if you have any other ideas on testing failover
 using
 rdma please let me know. I have successfully
 used tcp_bw tests for this purpose already.

 Thanks,
 Suri

 --
 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
  
   --
   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

--
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

RE: Qperf and APM

[Luick, Dean]

Hi,

Can you be more specific?  What are you running and how is it failing?

Thanks,
Dean

 -Original Message-
 From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
 ow...@vger.kernel.org] On Behalf Of Suresh Shelvapille
 Sent: Wednesday, March 06, 2013 9:40 AM
 To: linux-rdma@vger.kernel.org
 Cc: Hefty, Sean
 Subject: Qperf and APM
 
 Folks:
 The qperf utility has -ap, -lap, -rap options in support of APM. Unfortunately
 they don't seem to work. Is there a way
 to make this work, or if you have any other ideas on testing failover using
 rdma please let me know. I have successfully
 used tcp_bw tests for this purpose already.
 
 Thanks,
 Suri
 
 --
 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
--
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

RE: Qperf and APM

[Luick, Dean]

Have you tried ignoring the warning and checking of the alternate port works?  
The qperf code looks to be using the alternate port, but incorrectly fails to 
mark the argument as used.

Dean

 -Original Message-
 From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
 ow...@vger.kernel.org] On Behalf Of Suresh Shelvapille
 Sent: Wednesday, March 06, 2013 1:00 PM
 To: Luick, Dean; linux-rdma@vger.kernel.org
 Subject: RE: Qperf and APM
 
 This is the error I see on the client(the same warning is seen with -lap and -
 rap options):
 
 --
 qperf server-ip-addr -ap 2 rc_bw
 rc_bw:
 warning: -ap set but not used in test rc_bw
 bw  =  3.39 GB/sec
 
  -Original Message-
  From: Luick, Dean [mailto:dean.lu...@intel.com]
  Sent: Wednesday, March 06, 2013 1:52 PM
  To: Suresh Shelvapille; linux-rdma@vger.kernel.org
  Cc: Hefty, Sean
  Subject: RE: Qperf and APM
 
  Hi,
 
  Can you be more specific?  What are you running and how is it failing?
 
  Thanks,
  Dean
 
   -Original Message-
   From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
   ow...@vger.kernel.org] On Behalf Of Suresh Shelvapille
   Sent: Wednesday, March 06, 2013 9:40 AM
   To: linux-rdma@vger.kernel.org
   Cc: Hefty, Sean
   Subject: Qperf and APM
  
   Folks:
   The qperf utility has -ap, -lap, -rap options in support of APM.
 Unfortunately
   they don't seem to work. Is there a way
   to make this work, or if you have any other ideas on testing failover 
   using
   rdma please let me know. I have successfully
   used tcp_bw tests for this purpose already.
  
   Thanks,
   Suri
  
   --
   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
 
 --
 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
--
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

RE: crash in librdmacm

[Luick, Dean]

 -Original Message-
 Looking further, I also notice that 'enum rdma_cm_event_type' does not
 initialize a single value. AFAIK, according to the C standard at least
 the first value has to be initialized, otherwise the compiler is free to
 assign random values. This is certainly fine for in-kernel enums,
 however, as kernel and user space need to use the same numbers this
 probably should be improved. Shall I write a patch, assigning the first
 value to 0 for kernel and lib definitions?


In a draft of the C specification (all I have handy), section 6.5.2.2  
Enumeration specifiers, says:

... 
If the first enumerator has no =, the value  of its  enumeration  constant is 0.
...


Dean

RE: [PATCH libmlx4 1/8] Add raw packet QP support

[Luick, Dean]

 -Original Message-
 From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
 ow...@vger.kernel.org] On Behalf Of Or Gerlitz
 Sent: Thursday, September 20, 2012 3:31 PM
 To: rol...@kernel.org
 Cc: linux-rdma@vger.kernel.org; Or Gerlitz
 Subject: [PATCH libmlx4 1/8] Add raw packet QP support
 
 Implement raw packet QPs for Ethernet ports.
 
 Signed-off-by: Or Gerlitz ogerl...@mellanox.com
 ---
  src/qp.c |4 
  1 files changed, 4 insertions(+), 0 deletions(-)
 
 diff --git a/src/qp.c b/src/qp.c
 index 40a6689..90c4e80 100644
 --- a/src/qp.c
 +++ b/src/qp.c
 @@ -286,6 +286,10 @@ int mlx4_post_send(struct ibv_qp *ibqp, struct
 ibv_send_wr *wr,
   size += sizeof (struct mlx4_wqe_datagram_seg) / 16;
   break;
 
 + case IBV_QPT_RAW_PACKET:
 + /* For raw eth, the MLX4_WQE_CTRL_SOLICIT flag is
 used
 +  * to indicate that no icrc should be calculated */
 + ctrl-srcrb_flags |=
 htonl(MLX4_WQE_CTRL_SOLICIT);
   default:
   break;
   }

Add a break for the new case?  While the above code works as expected, it is 
inconsistent from what it was before, and could lead to problems if the default 
case starts doing something.


Dean
--
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