[openib-general] Re: [PATCH 13/16] ehca: firmware InfiniBand interface

[Kyle Moffett]

On Apr 27, 2006, at 08:37:01, Jörn Engel wrote:
> On Thu, 27 April 2006 12:49:36 +0200, Heiko J Schick wrote:
>> +u64 hipz_h_alloc_resource_qp(const struct ipz_adapter_handle
>> adapter_handle,
>> +			     struct ehca_pfqp *pfqp,
>> +			     const u8 servicetype,
>> +			     const u8 daqp_ctrl,
>> +			     const u8 signalingtype,
>> +			     const u8 ud_av_l_key_ctl,
>> +			     const struct ipz_cq_handle send_cq_handle,
>> +			     const struct ipz_cq_handle receive_cq_handle,
>> +			     const struct ipz_eq_handle async_eq_handle,
>> +			     const u32 qp_token,
>> +			     const struct ipz_pd pd,
>> +			     const u16 max_nr_send_wqes,
>> +			     const u16 max_nr_receive_wqes,
>> +			     const u8 max_nr_send_sges,
>> +			     const u8 max_nr_receive_sges,
>> +			     const u32 ud_av_l_key,
>> +			     struct ipz_qp_handle *qp_handle,
>> +			     u32 * qp_nr,
>> +			     u16 * act_nr_send_wqes,
>> +			     u16 * act_nr_receive_wqes,
>> +			     u8 * act_nr_send_sges,
>> +			     u8 * act_nr_receive_sges,
>> +			     u32 * nr_sq_pages,
>> +			     u32 * nr_rq_pages,
>> +			     struct h_galpas *h_galpas);
>
> 25 parameters?  If you tell me which drugs were involved in this  
> code, I know what to stay away from.  Might be the current record  
> for any code ever proposed for inclusion.
>
> The whole patch is full of parameter-happy functions with this one  
> being the ugly top of the iceberg.  I sincerely hope this is not a  
> defined ABI and can still be changed.

What's worse; look at the stack usage on that sucker alone:

10 pointers, 6 u8, 2 u16, 2 u32, and topped off with 3 unknown-sized  
"struct ipz_cq_handle", an unknown-sized "struct ipz_pd".  The  
alignment alone probably chews up at least another couple bytes in  
there somewhere too.  That's at _least_ 98 + 3*sizeof(struct  
ipz_cq_handle) + sizeof(struct ipz_pd) on a 64-bit platform (58 +  
3*sizeof(struct ipz_cq_handle) + sizeof(struct ipz_pd) on 32-bit).   
Not to mention the fact that you totally screwed the compiler's  
chances of ever passing the important stuff in registers.  And you  
haven't even gotten into local variables yet.

Cheers,
Kyle Moffett