Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

2006-09-06 Thread Jan-Bernd Themann
Hi,

ok, I admit this solution looks a bit nicer. We changed it in a similar way.

Jan-Bernd

On Tuesday 05 September 2006 20:58, Francois Romieu wrote:
 Thomas Klein [EMAIL PROTECTED] :
 [...]
  Somehow I don't get your point concerning the usage of 'k'. We need another
  iterator as the for loops using 'k' use 'i' as their terminating condition.
 
 Something like the code below perhaps (with more local variables maybe):
 
 static int ehea_reg_interrupts(struct net_device *dev)
 {
   struct ehea_port *port = netdev_priv(dev);
   struct ehea_port_res *pr;
   int i, ret;
 
   for (i = 0; i  port-num_def_qps; i++) {
   pr = port-port_res[i];
   snprintf(pr-int_recv_name, EHEA_IRQ_NAME_SIZE - 1
, %s-recv%d, dev-name, i);
   ret = ibmebus_request_irq(NULL, pr-recv_eq-attr.ist1,
 ehea_recv_irq_handler, SA_INTERRUPT,
 pr-int_recv_name, pr);
   if (ret) {
   ehea_error(failed registering irq for ehea_recv_int:
  port_res_nr:%d, ist=%X, i,
  pr-recv_eq-attr.ist1);
   goto err_free_irq_recv_eq_0;
   }
   if (netif_msg_ifup(port))
   ehea_info(irq_handle 0x%X for funct ehea_recv_int %d 
 registered, pr-recv_eq-attr.ist1, i);
   }
 
   snprintf(port-int_aff_name, EHEA_IRQ_NAME_SIZE - 1,
%s-aff, dev-name);
   ret = ibmebus_request_irq(NULL, port-qp_eq-attr.ist1,
 ehea_qp_aff_irq_handler,
 SA_INTERRUPT, port-int_aff_name, port);
   if (ret) {
   ehea_error(failed registering irq for qp_aff_irq_handler:
   ist=%X, port-qp_eq-attr.ist1);
   goto err_free_irq_recv_eq_0;
   }
   if (netif_msg_ifup(port))
   ehea_info(irq_handle 0x%X for function qp_aff_irq_handler 
 registered, port-qp_eq-attr.ist1);
 
   for (i = 0; i  port-num_def_qps + port-num_add_tx_qps; i++) {
   pr = port-port_res[i];
   snprintf(pr-int_send_name, EHEA_IRQ_NAME_SIZE - 1,
%s-send%d, dev-name, i);
   ret = ibmebus_request_irq(NULL, pr-send_eq-attr.ist1,
 ehea_send_irq_handler, SA_INTERRUPT,
 pr-int_send_name, pr);
   if (ret) {
   ehea_error(failed registering irq for ehea_send
   port_res_nr:%d, ist=%X, i,
  pr-send_eq-attr.ist1);
   goto err_free_irq_send_eq_1;
   }
   if (netif_msg_ifup(port))
   ehea_info(irq_handle 0x%X for function ehea_send_int 
 %d registered, pr-send_eq-attr.ist1, i);
   }
 out:
   return ret;
 
 err_free_irq_send_eq_1:
   // Post-dec works with unsigned int too.
   while (i--  0) {
   u32 ist = port-port_res[i].send_eq-attr.ist1;
   ibmebus_free_irq(NULL, ist, port-port_res[i]);
   }
   ibmebus_free_irq(NULL, port-qp_eq-attr.ist1, port);
   i = port-num_def_qps;
 err_free_irq_recv_eq_0:
   while (i--  0) {
   u32 ist = port-port_res[i].recv_eq-attr.ist1;
   ibmebus_free_irq(NULL, ist, port-port_res[k]);
   }
   goto out;
 }
 
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

2006-09-06 Thread Jeff Garzik

Arnd Bergmann wrote:

Am Monday 04 September 2006 22:16 schrieb Francois Romieu:

+#include ehea.h
+#include ehea_qmr.h
+#include ehea_phyp.h

Afaik none of those is included in this patch nor in my 2.6.18-git tree.



They are in 5, 3 and 2, respectively


Happy bissect in sight.


The driver should get merged as a single commit anyway, even
if split diffs are posted for review. Even if it gets merged
like this, bisect will work since the Kconfig option is added
in the final patch.


That's impossible, in the form they were submitted...


-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

2006-09-05 Thread Thomas Klein

Hi Francois,

thanks for your review and your comments. See below our answers.

Regards
Thomas



Francois Romieu wrote:

 +cb2 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
 +if (!cb2) {
 +ehea_error(no mem for cb2);
 +goto kzalloc_failed;

 It's better when the label tell what it does than where it comes from.
 If it's numbered too, one can check them without going back and forth.
 +stats-tx_packets = cb2-txucp + cb2-txmcp + cb2-txbcp;
 +stats-multicast = cb2-rxmcp;
 +stats-rx_errors = cb2-rxuerr;
 +stats-rx_bytes = cb2-rxo;
 +stats-tx_bytes = cb2-txo;
 +stats-rx_packets = rx_packets;
 +
 +hcall_failed:
 +kfree(cb2);

 Tab was turned into spaces.

Fixed.

 +static inline int ehea_refill_rq1(struct ehea_port_res *pr, int index,

 Avoid inline ?

Inline declaration was removed from this one and several other functions.

 +for (i = 0; i  nr_of_wqes; i++) {
 +if (!skb_arr_rq1[index]) {
 +skb_arr_rq1[index] = dev_alloc_skb(EHEA_LL_PKT_SIZE);

 netdev_alloc_skb ?

Agreed  done.


 +
 +if (!skb_arr_rq1[index]) {
 +ehea_error(no mem for skb/%d wqes filled, i);
 +ret = -ENOMEM;

 The caller does not check the returned value.

Agreed. fn returns void now.

 +if (!skb_arr_rq1[i]) {
 +ehea_error(no mem for skb/%d skbs filled., i);
 +ret = -ENOMEM;
 +goto exit0;

 s/exit0/out/

Goto target naming was reworked throughout the whole driver and basically
uses the style used by Dave M. and Jeff G. in the Tigon3 driver.

 +static inline int ehea_check_cqe(struct ehea_cqe *cqe, int *rq_num)
 +{
 +*rq_num = (cqe-type  EHEA_CQE_TYPE_RQ)  5;
 +if ((cqe-status  EHEA_CQE_STAT_ERR_MASK) == 0)
 +return 0;
 +if (((cqe-status  EHEA_CQE_STAT_ERR_TCP) != 0)
 + (cqe-header_length == 0))

  on the previous line please.

Changed at all occurences.

 +static inline struct sk_buff *get_skb_by_index(struct sk_buff **skb_array,
 +   int arr_len,
 +   struct ehea_cqe *cqe)
 +{
 +int skb_index = EHEA_BMASK_GET(EHEA_WR_ID_INDEX, cqe-wr_id);
 +struct sk_buff *skb;
 +void *pref;
 +int x;
 +
 +x = skb_index + 1;
 +x = (arr_len - 1);
 +
 +pref = (void*)skb_array[x];

 Useless cast.

Agreed - removed.

 +if (unlikely(!skb)) {
 +if (netif_msg_rx_err(port))
 +ehea_error(LL rq1: skb=NULL);
 +skb = 
dev_alloc_skb(EHEA_LL_PKT_SIZE);

 Tab/space

Fixed.

 +irqreturn_t ehea_qp_aff_irq_handler(int irq, void *param, struct pt_regs * 
regs)

 static ?

Agreed.

 +int ehea_sense_port_attr(struct ehea_port *port)

 static ?

No - used in ehea_ethtool.c

 +} else {
 +if (hret == H_AUTHORITY)
 +{

 Misplaced curly brace.

Fixed.


 +ehea_info(Hypervisor denied setting port speed. Either
 +   this partition is not authorized to set 
 +  port speed or another partition has modified
 +   port speed first.);
 +ret = -EPERM;
 +} else
 +{

 Misplaced curly brace.

Fixed.


 +ret = -EIO;
 +ehea_error(Failed setting port speed);
 +}
 +}
 +netif_carrier_on(port-netdev);
 +exit0:
 +kfree(cb4);

 cb4 is NULL. Not wrong per se but I'd rather move the label one line down.

Agreed.

 +void ehea_neq_tasklet(unsigned long data)

 static ?

Agreed.

 +irqreturn_t ehea_interrupt_neq(int irq, void *param, struct pt_regs *regs)

 static ?

Agreed.

 +{
 +struct ehea_adapter *adapter = (struct ehea_adapter*)param;

 Useless cast.

Fixed.

 +static int ehea_fill_port_res(struct ehea_port_res *pr)
 +{
 +int ret;
 +struct ehea_qp_init_attr *init_attr = pr-qp-init_attr;
 +
 +/* RQ 1 */
 +ret = ehea_init_fill_rq1(pr, init_attr-act_nr_rwqes_rq1
 + - init_attr-act_nr_rwqes_rq2
 + - init_attr-act_nr_rwqes_rq3 - 1);
 +/* RQ 2 */

 Useless comment.

Removed.

 +for (k = 0; k  i; k++) {
 +u32 ist = port-port_res[k].recv_eq-attr.ist1;
 +ibmebus_free_irq(NULL, ist, port-port_res[k]);
 +}
 +goto failure;

 Poor label (and bloaty release practice too: remove k, reuse i below
 and more importantly release the things in allocation-reversed order).

Somehow I don't get your point concerning the usage of 'k'. We need another
iterator as the for loops using 'k' use 'i' as their terminating condition.


 +}
 +if 

Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

2006-09-05 Thread Francois Romieu
Thomas Klein [EMAIL PROTECTED] :
[...]
 Somehow I don't get your point concerning the usage of 'k'. We need another
 iterator as the for loops using 'k' use 'i' as their terminating condition.

Something like the code below perhaps (with more local variables maybe):

static int ehea_reg_interrupts(struct net_device *dev)
{
struct ehea_port *port = netdev_priv(dev);
struct ehea_port_res *pr;
int i, ret;

for (i = 0; i  port-num_def_qps; i++) {
pr = port-port_res[i];
snprintf(pr-int_recv_name, EHEA_IRQ_NAME_SIZE - 1
 , %s-recv%d, dev-name, i);
ret = ibmebus_request_irq(NULL, pr-recv_eq-attr.ist1,
  ehea_recv_irq_handler, SA_INTERRUPT,
  pr-int_recv_name, pr);
if (ret) {
ehea_error(failed registering irq for ehea_recv_int:
   port_res_nr:%d, ist=%X, i,
   pr-recv_eq-attr.ist1);
goto err_free_irq_recv_eq_0;
}
if (netif_msg_ifup(port))
ehea_info(irq_handle 0x%X for funct ehea_recv_int %d 
  registered, pr-recv_eq-attr.ist1, i);
}

snprintf(port-int_aff_name, EHEA_IRQ_NAME_SIZE - 1,
 %s-aff, dev-name);
ret = ibmebus_request_irq(NULL, port-qp_eq-attr.ist1,
  ehea_qp_aff_irq_handler,
  SA_INTERRUPT, port-int_aff_name, port);
if (ret) {
ehea_error(failed registering irq for qp_aff_irq_handler:
ist=%X, port-qp_eq-attr.ist1);
goto err_free_irq_recv_eq_0;
}
if (netif_msg_ifup(port))
ehea_info(irq_handle 0x%X for function qp_aff_irq_handler 
  registered, port-qp_eq-attr.ist1);

for (i = 0; i  port-num_def_qps + port-num_add_tx_qps; i++) {
pr = port-port_res[i];
snprintf(pr-int_send_name, EHEA_IRQ_NAME_SIZE - 1,
 %s-send%d, dev-name, i);
ret = ibmebus_request_irq(NULL, pr-send_eq-attr.ist1,
  ehea_send_irq_handler, SA_INTERRUPT,
  pr-int_send_name, pr);
if (ret) {
ehea_error(failed registering irq for ehea_send
port_res_nr:%d, ist=%X, i,
   pr-send_eq-attr.ist1);
goto err_free_irq_send_eq_1;
}
if (netif_msg_ifup(port))
ehea_info(irq_handle 0x%X for function ehea_send_int 
  %d registered, pr-send_eq-attr.ist1, i);
}
out:
return ret;

err_free_irq_send_eq_1:
// Post-dec works with unsigned int too.
while (i--  0) {
u32 ist = port-port_res[i].send_eq-attr.ist1;
ibmebus_free_irq(NULL, ist, port-port_res[i]);
}
ibmebus_free_irq(NULL, port-qp_eq-attr.ist1, port);
i = port-num_def_qps;
err_free_irq_recv_eq_0:
while (i--  0) {
u32 ist = port-port_res[i].recv_eq-attr.ist1;
ibmebus_free_irq(NULL, ist, port-port_res[k]);
}
goto out;
}
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

2006-09-04 Thread Arnd Bergmann
Am Monday 04 September 2006 22:16 schrieb Francois Romieu:
  +#include ehea.h
  +#include ehea_qmr.h
  +#include ehea_phyp.h

 Afaik none of those is included in this patch nor in my 2.6.18-git tree.


They are in 5, 3 and 2, respectively

 Happy bissect in sight.

The driver should get merged as a single commit anyway, even
if split diffs are posted for review. Even if it gets merged
like this, bisect will work since the Kconfig option is added
in the final patch.

  +
  +static struct net_device_stats *ehea_get_stats(struct net_device *dev)
  +{
  + struct ehea_port *port = netdev_priv(dev);
  + struct net_device_stats *stats = port-stats;
  + struct hcp_ehea_port_cb2 *cb2;
  + u64 hret, rx_packets;
  + int i;

 unsigned int ?

does it matter? int as a counter is pretty standard.

  +
  + if (netif_msg_hw(port))
  + ehea_dump(cb2, sizeof(*cb2), net_device_stats);
  +
  + rx_packets = 0;

 Could be initialized when it is declared.

  + for (i = 0; i  port-num_def_qps; i++)
  + rx_packets += port-port_res[i].rx_packets;

In one of the previous reviews, we told them to do it this way
instead. Initializing at declaration is error-prone.

  +
  + intreq = ((pr-p_state.ehea_poll  0xF) == 0xF);

 Arguable parenthesis.


I'd argue to keep them ;-)

  +
  + hret = ehea_h_modify_ehea_port(port-adapter-handle,
  +        port-logical_port_id,
  +        H_PORT_CB0, mask, cb0);
  + if (hret != H_SUCCESS) {
  + ret = -EIO;

 Why can't ehea_xyz return -EIO/0 directly ?


the lowest-level hypercall should return H_* by convention.

Then again, it should also be called plpar_modify_ehea_port()
in that case.

  +static int ehea_start_xmit(struct sk_buff *skb, struct net_device *dev)
  +{
  + struct ehea_port *port = netdev_priv(dev);
  + struct ehea_port_res *pr;
  + struct ehea_swqe *swqe;
  + unsigned long flags;
  + u32 lkey;
  + int swqe_index;
  +
  + pr = port-port_res[0];

 Initialization and declaration can happen at the same time.

it's a gray area. In general, I recommend not to combine them
at all. Initialization to NULL or 0 is always bad, this one
is harmless, but I'd still leave it this way, especially after
telling them to clean this up earlier ;-)

Arnd 
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

2006-09-04 Thread Francois Romieu
Arnd Bergmann [EMAIL PROTECTED] :
[...]
 The driver should get merged as a single commit anyway, even
 if split diffs are posted for review. Even if it gets merged
 like this, bisect will work since the Kconfig option is added
 in the final patch.

I have seen/done worse but it's not exactly pretty.

[...]
   +?int i;
 
  unsigned int ?
 
 does it matter? int as a counter is pretty standard.

ppc64 takes unsigned int as a little optimization. I do not know how
the target platform behaves here.

-- 
Ueimor
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

2006-08-21 Thread Jan-Bernd Themann
Hi

On Friday 18 August 2006 16:44, Alexey Dobriyan wrote:
  +static int ehea_init_port_res(struct ehea_port *port, struct ehea_port_res 
  *pr,
  + struct port_res_cfg *pr_cfg, int queue_token)
  +{
  +   int ret = -EINVAL;
  +   int max_rq_entries = 0;
  +   enum ehea_eq_type eq_type = EHEA_EQ;
  +   struct ehea_qp_init_attr *init_attr = NULL;
  +   struct ehea_adapter *adapter = port-adapter;
  +
  +   memset(pr, 0, sizeof(struct ehea_port_res));
  +
  +   pr-skb_arr_rq3 = NULL;
  +   pr-skb_arr_rq2 = NULL;
  +   pr-skb_arr_rq1 = NULL;
  +   pr-skb_arr_sq = NULL;
  +   pr-qp = NULL;
  +   pr-send_cq = NULL;
  +   pr-recv_cq = NULL;
  +   pr-send_eq = NULL;
  +   pr-recv_eq = NULL;
 
 After memset unneeded. ;-)
 

Is it valid (common in the kernel environment) to treat NULL as 0 after a memset
and thus to forget about initialization?

Thanks,
Jan-Bernd
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

2006-08-21 Thread Jörn Engel
On Mon, 21 August 2006 14:23:53 +0200, Jan-Bernd Themann wrote:
 
 Is it valid (common in the kernel environment) to treat NULL as 0 after a 
 memset
 and thus to forget about initialization?

Yes.  According to C99, An implementation might conveivably have
codes for floating zero and/or null pointer other than all bits zero.
But as long as you don't use floating point (you shouldn't) and don't
redefine NULL to something other than (void*)0, this is rather
inconceivable for the kernel.

Jörn

-- 
Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it.
-- Brian W. Kernighan
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

2006-08-21 Thread Thomas Klein

Alexey Dobriyan wrote:
 On Fri, Aug 18, 2006 at 01:29:01PM +0200, Jan-Bernd Themann wrote:
 --- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea_main.c
 +++ kernel/drivers/net/ehea/ehea_main.c

 +static inline int ehea_refill_rq3_def(struct ehea_port_res *pr, int 
nr_of_wqes)

 This one looks too big to be inlined as well as other similalry named
 functions.

Agreed. Inlining avoided where possible.

 +static int ehea_clean_port_res(struct ehea_port *port, struct ehea_port_res 
*pr)

 Freeing/deallocating/cleaning functions usually return void. The fact
 that you always return -EINVAL only reaffirmes my belief. ;-)


Fixed.

 +static inline void write_swqe2_data(struct sk_buff *skb,
 +struct net_device *dev,
 +struct ehea_swqe *swqe,
 +u32 lkey)

 Way too big.

Function split.

 +static inline void ehea_xmit2(struct sk_buff *skb,
 +  struct net_device *dev, struct ehea_swqe *swqe,
 +  u32 lkey)
 +
 +static inline void ehea_xmit3(struct sk_buff *skb,
 +  struct net_device *dev, struct ehea_swqe *swqe)

 Ditto.

These functions are on a performance-critical path and they are called
exactly once - so the object's size isn't affected and having them inline
seems appropriate to me.

 +if (grp)
 +memset(cb1-vlan_filter, 0, sizeof(cb1-vlan_filter));
 +else
 +memset(cb1-vlan_filter, 1, sizeof(cb1-vlan_filter));

 Just to be sure, this should be 1 not 0xff?


Will be checked.

 +void ehea_clean_all_port_res(struct ehea_port *port)
 +{
 +int ret;
 +int i;
 +for(i = 0; i  port-num_def_qps + port-num_tx_qps; i++)
 +ehea_clean_port_res(port, port-port_res[i]);
 +
 +ret = ehea_destroy_eq(port-qp_eq);
 +}

 ret is entirely useless.

Correct. It's gone.


 +int __init ehea_module_init(void)
 static

 +{
 +int ret = -EINVAL;
 +
 +printk(IBM eHEA Ethernet Device Driver (Release %s)\n, DRV_VERSION);
 +
 +ret = ibmebus_register_driver(ehea_driver);
 +if (ret) {
 +ehea_error(failed registering eHEA device driver on ebus);
 +return -EINVAL;
 +}
 +
 +return 0;
 +}

 Pass ret to upper layer. Simplest way is:

   static int __init ehea_module_init(void)
   {
   return ibmebus_register_driver(ehea_driver);
   }

Agreed to pass ret to upper layer, but we want to keep the error message.
Code modified accordingly.


Regards
Thomas
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

2006-08-18 Thread Alexey Dobriyan
On Fri, Aug 18, 2006 at 01:29:01PM +0200, Jan-Bernd Themann wrote:

Was there noticeable performance difference when explicit prefetching is
removed? At some (invisible) point CPUs will become smarter about prefetching
than programmers and this code will be slower than possible.

 +static inline struct sk_buff *get_skb_by_index(struct sk_buff **skb_array,
 +int arr_len,
 +struct ehea_cqe *cqe)
 +{
 + struct sk_buff *skb;
 + void *pref;
 + int x;
 + int skb_index = EHEA_BMASK_GET(EHEA_WR_ID_INDEX, cqe-wr_id);
 +
 + x = skb_index + 1;
 + x = (arr_len - 1);
 +
 + pref = (void*)skb_array[x];
 + prefetchw(pref);
 + prefetchw(pref + EHEA_CACHE_LINE);
 +
 + pref = (void*)(skb_array[x]-data);
 + prefetch(pref);
 + prefetch(pref + EHEA_CACHE_LINE);
 + prefetch(pref + EHEA_CACHE_LINE * 2);
 + prefetch(pref + EHEA_CACHE_LINE * 3);
 + skb = skb_array[skb_index];
 + skb_array[skb_index] = NULL;
 + return skb;
 +}
 +
 +static inline struct sk_buff *get_skb_by_index_ll(struct sk_buff **skb_array,
 +   int arr_len, int wqe_index)
 +{
 + struct sk_buff *skb;
 + void *pref;
 + int x;
 +
 + x = wqe_index + 1;
 + x = (arr_len - 1);
 +
 + pref = (void*)skb_array[x];
 + prefetchw(pref);
 + prefetchw(pref + EHEA_CACHE_LINE);
 +
 + pref = (void*)(skb_array[x]-data);
 + prefetchw(pref);
 + prefetchw(pref + EHEA_CACHE_LINE);
 +
 + skb = skb_array[wqe_index];
 + skb_array[wqe_index] = NULL;
 + return skb;
 +}

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

2006-08-18 Thread Arjan van de Ven
On Fri, 2006-08-18 at 17:47 +0400, Alexey Dobriyan wrote:
 On Fri, Aug 18, 2006 at 01:29:01PM +0200, Jan-Bernd Themann wrote:
 
 Was there noticeable performance difference when explicit prefetching is
 removed? At some (invisible) point CPUs will become smarter about prefetching
 than programmers and this code will be slower than possible.

Hi,

what you say is true in general, however the packet response part of the
kernel is special; the cpu just cannot know where your card just dma'd
the packet to (and thus cleared it from any caches anywhere in the
system) so it's going to be pretty much a 100% cachemiss always...

Greetings,
   Arjan van de Ven

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

2006-08-18 Thread Alexey Dobriyan
On Fri, Aug 18, 2006 at 01:29:01PM +0200, Jan-Bernd Themann wrote:
 --- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea_main.c
 +++ kernel/drivers/net/ehea/ehea_main.c

 +static struct net_device_stats *ehea_get_stats(struct net_device *dev)
 +{
 + int i;
 + u64 hret = H_HARDWARE;

Useless assignment here and everywhere.

 + u64 rx_packets = 0;
 + struct ehea_port *port = netdev_priv(dev);
 + struct hcp_query_ehea_port_cb_2 *cb2 = NULL;
 + struct net_device_stats *stats = port-stats;
 +
 + cb2 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
 + if (!cb2) {
 + ehea_error(no mem for cb2);
 + goto get_stat_exit;
 + }
 +
 + hret = ehea_h_query_ehea_port(port-adapter-handle,
 +   port-logical_port_id,
 +   H_PORT_CB2,
 +   H_PORT_CB2_ALL,
 +   cb2);

 +static inline int ehea_refill_rq1(struct ehea_port_res *pr, int index,
 +   int nr_of_wqes)
 +{
 + int i;
 + int ret = 0;
 + struct sk_buff **skb_arr_rq1 = pr-skb_arr_rq1;
 + int max_index_mask = pr-skb_arr_rq1_len - 1;
 +
 + if (!nr_of_wqes)
 + return 0;
 +
 + for (i = 0; i  nr_of_wqes; i++) {
 + if (!skb_arr_rq1[index]) {
 + skb_arr_rq1[index] = dev_alloc_skb(EHEA_LL_PKT_SIZE);
 +
 + if (!skb_arr_rq1[index]) {
 + ehea_error(no mem for skb/%d wqes filled, i);
 + ret = -ENOMEM;
 + break;
 + }
 + }
 + index--;
 + index = max_index_mask;
 + }
 + /* Ring doorbell */
 + ehea_update_rq1a(pr-qp, i);
 +
 + return ret;
 +}
 +
 +static int ehea_init_fill_rq1(struct ehea_port_res *pr, int nr_rq1a)
 +{
 + int i;
 + int ret = 0;
 + struct sk_buff **skb_arr_rq1 = pr-skb_arr_rq1;
 +
 + for (i = 0; i  pr-skb_arr_rq1_len; i++) {
 + skb_arr_rq1[i] = dev_alloc_skb(EHEA_LL_PKT_SIZE);
 + if (!skb_arr_rq1[i]) {
 + ehea_error(no mem for skb/%d skbs filled., i);
 + ret = -ENOMEM;
 + goto nomem;
 + }
 + }
 + /* Ring doorbell */
 + ehea_update_rq1a(pr-qp, nr_rq1a);
 +nomem:
 + return ret;
 +}
 +
 +static inline int ehea_refill_rq2_def(struct ehea_port_res *pr, int 
 nr_of_wqes)
 +{
 + int i;
 + int ret = 0;
 + struct ehea_qp *qp;
 + struct ehea_rwqe *rwqe;
 + struct sk_buff **skb_arr_rq2 = pr-skb_arr_rq2;
 + int index, max_index_mask;
 +
 + if (!nr_of_wqes)
 + return 0;
 +
 + qp = pr-qp;
 +
 + index = pr-skb_rq2_index;
 + max_index_mask = pr-skb_arr_rq2_len - 1;
 + for (i = 0; i  nr_of_wqes; i++) {
 + struct sk_buff *skb = dev_alloc_skb(EHEA_RQ2_PKT_SIZE
 + + NET_IP_ALIGN);
 + if (!skb) {
 + ehea_error(no mem for skb/%d wqes filled, i);
 + ret = -ENOMEM;
 + break;
 + }
 + skb_reserve(skb, NET_IP_ALIGN);
 +
 + skb_arr_rq2[index] = skb;
 +
 + rwqe = ehea_get_next_rwqe(qp, 2);
 + rwqe-wr_id = EHEA_BMASK_SET(EHEA_WR_ID_TYPE, EHEA_RWQE2_TYPE)
 + | EHEA_BMASK_SET(EHEA_WR_ID_INDEX, index);
 + rwqe-sg_list[0].l_key = pr-recv_mr.lkey;
 + rwqe-sg_list[0].vaddr = (u64)skb-data;
 + rwqe-sg_list[0].len = EHEA_RQ2_PKT_SIZE;
 + rwqe-data_segments = 1;
 +
 + index++;
 + index = max_index_mask;
 + }
 + pr-skb_rq2_index = index;
 +
 + /* Ring doorbell */
 + iosync();
 + ehea_update_rq2a(qp, i);
 + return ret;
 +}
 +
 +
 +static inline int ehea_refill_rq2(struct ehea_port_res *pr, int nr_of_wqes)
 +{
 + return ehea_refill_rq2_def(pr, nr_of_wqes);
 +}
 +
 +static inline int ehea_refill_rq3_def(struct ehea_port_res *pr, int 
 nr_of_wqes)
 +{
 + int i;
 + int ret = 0;
 + struct ehea_qp *qp = pr-qp;
 + struct ehea_rwqe *rwqe;
 + int skb_arr_rq3_len = pr-skb_arr_rq3_len;
 + struct sk_buff **skb_arr_rq3 = pr-skb_arr_rq3;
 + int index, max_index_mask;
 +
 + if (nr_of_wqes == 0)
 + return -EINVAL;
 +
 + index = pr-skb_rq3_index;
 + max_index_mask = skb_arr_rq3_len - 1;
 + for (i = 0; i  nr_of_wqes; i++) {
 + struct sk_buff *skb = dev_alloc_skb(EHEA_MAX_PACKET_SIZE
 + + NET_IP_ALIGN);
 + if (!skb) {
 + ehea_error(no mem for skb/%d wqes filled, i);
 + ret = -ENOMEM;
 + break;
 + }
 + skb_reserve(skb, NET_IP_ALIGN);
 +
 + rwqe = ehea_get_next_rwqe(qp, 3);
 

Re: [PATCH 1/7] ehea: interface to network stack

2006-08-15 Thread Heiko Carstens
 +int __init ehea_module_init(void)
 +{
 + int ret = -EINVAL;
 +
 + EDEB_EN(7, );
 +
 + printk(KERN_INFO IBM eHEA Ethernet Device Driver (Release %s)\n,
 +DRV_VERSION);
 +
 +
 + ret = ibmebus_register_driver(ehea_driver);
 + if (ret) {
 + EDEB_ERR(4, Failed registering eHEA device driver on ebus);
 + return -EINVAL;
 + }
 +
 + EDEB_EX(7, );
 + return 0;
 +}

Function should be static and could be shortened to the single line

return ibmebus_register_driver(ehea_driver);

, I guess :)
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html