<html><body>
<p><tt>general-bounces@lists.openfabrics.org wrote on 02/05/2008 04:44:09 AM:<br>
<br>
> Hi Shirley,<br>
> <br>
> I see the following problems with the patch:<br>
> <br>
> 1.static inline void ipoib_sg_dma_unmap_rx(struct ipoib_dev_priv *priv,<br>
> + u64 mapping[IPOIB_UD_RX_SG])<br>
> +{<br>
> + ib_dma_unmap_single(priv->ca, mapping[0], <br>
> IPOIB_UD_HEAD_SIZE, DMA_FROM_DEVICE);<br>
> <br>
> ==> use ib_dma_unmap_page()<br>
> + ib_dma_unmap_single(priv->ca, mapping[1], PAGE_SIZE, DMA_FROM_DEVICE);<br>
> +}</tt><br>
<br>
<tt>Good catch, I will fix it.</tt><br>
<tt> <br>
> 2. When you allocate an SKB with ipoib_sg_alloc_rx_skb(), you allocate<br>
> and map both the linear data and the first fragment (in the case of 4K<br>
> mtu and 4K page size). But then you call ipoib_ud_skb_put_frags() to<br>
> potentially take the first fragment from the SKB for which a packet has<br>
> just received. This can cause a leak of one page (although I think this<br>
> case should never happen since the the length of packet is likely to<br>
> exceed the linear data of the SKB.<br>
</tt><br>
<tt>The first buffer only has GRH + IPoIB header = 44 bytes, the first fragment has the IP payload, the length can never be zero. Actually I can remove that code from taking the first fragment. It's useless here.</tt><br>
<tt> <br>
> 3. The <br>
> if (ipoib_ud_need_sg(priv->max_ib_mtu)) {<br>
> <br>
> in ipoib_ib_handle_rx_wc() can be eliminated - most of the code is<br>
> identical.<br>
</tt><br>
<tt>The skb free and allocation need this check, the rest of code can share between 2K and 4K. I am trying to use one condition check here. Can you please tell me how to totally eliminate it in details?</tt><br>
<br>
<tt>thanks</tt><br>
<tt>Shirley</tt></body></html>