[ofa-general] 4k MTU patch

[Eli Cohen]

On Tue, 2008-02-05 at 07:34 -0800, Shirley Ma wrote:
> general-bounces at lists.openfabrics.org wrote on 02/05/2008 04:44:09 AM:
> 
> > Hi Shirley,
> > 
> > I see the following problems with the patch:
> > 
> > 1.static inline void ipoib_sg_dma_unmap_rx(struct ipoib_dev_priv
> *priv,
> > +                                        u64
> mapping[IPOIB_UD_RX_SG])
> > +{
> > +       ib_dma_unmap_single(priv->ca, mapping[0], 
> > IPOIB_UD_HEAD_SIZE, DMA_FROM_DEVICE);
> > 
> > ==> use ib_dma_unmap_page()
> > +       ib_dma_unmap_single(priv->ca, mapping[1], PAGE_SIZE,
> DMA_FROM_DEVICE);
> > +}
> 
> Good catch, I will fix it.
>  
> > 2. When you allocate an SKB with ipoib_sg_alloc_rx_skb(), you
> allocate
> > and map both the linear data and the first fragment (in the case of
> 4K
> > mtu and 4K page size). But then you call ipoib_ud_skb_put_frags() to
> > potentially take the first fragment from the SKB for which a packet
> has
> > just received. This can cause a leak of one page (although I think
> this
> > case should never happen since the the length of packet is likely to
> > exceed the linear data of the SKB.
> 
> 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.
>  
> > 3. The 
> > if (ipoib_ud_need_sg(priv->max_ib_mtu)) {
> > 
> > in ipoib_ib_handle_rx_wc() can be eliminated - most of the code is
> > identical.
> 
> 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?
> 
I think it would be better to use a single alloc and free for the
receive SKB and do the if inside those functions. For example, you can
do:

+static inline void ipoib_ud_dma_unmap_rx(struct ipoib_dev_priv *priv,
+                                        u64 *mapping)
+{
+       if (ipoib_ud_need_sg(priv->max_ib_mtu) {
+               ib_dma_unmap_single(priv->ca, mapping[0], IPOIB_UD_HEAD_SIZE, DMA_FROM_DEVICE);
+               ib_dma_unmap_page(priv->ca, mapping[1], PAGE_SIZE, DMA_FROM_DEVICE);
+       } else
+               ib_dma_unmap_single(priv->ca, mapping[0],
+                                   IPOIB_UD_BUF_SIZE(priv->max_ib_mtu),
+                                   DMA_FROM_DEVICE);
+}

and something similar for allocating.