[ofa-general] Re: [PATCH] IPoIB 4K MTU support

[Shirley Ma]

Hello Roland,

On Tue, 2008-04-22 at 13:46 -0700, Roland Dreier wrote:
> Thanks, applied with some cleanups as below.
Thanks!

> As an aside, in the case where we need to use a fragment in the receive
> skb, does it make sense to make the initial linear part bigger so the
> TCP and IP headers fit there (and the kernel doesn't have to look into
> the fragment list to handle the packet)?
We can improve this later.

> Also, is there any clean way where a kernel with PAGE_SIZE > 4096 can
> have ud_need_sg evaluate to 0 at compile time, so that all the unneeded
> code can be thrown out by the compiler?
> 
>  > +	return (IPOIB_UD_BUF_SIZE(ib_mtu) > PAGE_SIZE) ? 1 : 0;
> 
> I've never understood this style: it makes no sense to do
> 
> 	return bool ? 1 : 0;
> 
> instead of just
> 
> 	return bool;
You are right.

>  > +static inline void ipoib_ud_dma_unmap_rx(struct ipoib_dev_priv *priv,
>  > +					 u64 mapping[IPOIB_UD_RX_SG])
>  > +{
>  > +	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);
>  > +}
>  > +
>  > +static inline void ipoib_ud_skb_put_frags(struct ipoib_dev_priv *priv,
>  > +					  struct sk_buff *skb,
>  > +					  unsigned int length)
>  > +{
>  > +	if (ipoib_ud_need_sg(priv->max_ib_mtu)) {
>  > +		skb_frag_t *frag = &skb_shinfo(skb)->frags[0];
>  > +		/*
>  > +		 * There is only two buffers needed for max_payload = 4K,
>  > +		 * first buf size is IPOIB_UD_HEAD_SIZE
>  > +		 */
>  > +		skb->tail += IPOIB_UD_HEAD_SIZE;
>  > +		frag->size = length - IPOIB_UD_HEAD_SIZE;
>  > +		skb->data_len += frag->size;
>  > +		skb->truesize += frag->size;
>  > +		skb->len += length;
>  > +	} else
>  > +		skb_put(skb, length);
>  > +
>  > +}
> 
> These are pretty big to put in a header file as inlines... I moved them
> to the only .c file where they're used.
> 
>  - R.
Right. I should have moved it into .c file from Or's comment. I forgot. 

Thanks.
Shirley