Re: [PATCH] Gigabit Ethernet driver of Topcliff PCH

[Masayuki Ohtake]

Hi Eric and Stephen

Thank you for your suggestion.

I tried modification of "dev->hard_header_len".
However, there was no changed to a format of SKB received by kernel.
  [Header:14octet] + [payload]

So, I can not a single memcpy.


>Something like:
>       skb = skb_realloc_headroom(skb, 2);
>       if (!skb)
>           goto drop;
>__skb_push(skb, 2);
>        memmove(skb->data, skb->data + 2, ETH_HLEN);
>
>skb->data[ETH_HLEN] = 0;
>skb->data[ETH_HLEN+1] = 0;
>
>        if (!IS_ALIGNED(skb->data, 16)) {
>    nskb = netdev_alloc_skb(dev, skb->len + 16);
>    if (!nskb)
>                goto drop2;
>            skb_reserve(nskb, PTR_ALIGN(skb->data, 16) - skb->data);
>    skb_put(skb, skb->len);
>            memcpy(nskb->data, skb->data, skb->len);
>    dev_kfree_skb(skb);
>            skb = nskb;
>        }

I am going to try modification using "memmove()" and "IS_ALIGNED()".

Thanks, Ohtake(OKISemi)
----- Original Message ----- 
From: "Stephen Hemminger" <shemminger@xxxxxxxxxx>
To: "Eric Dumazet" <eric.dumazet@xxxxxxxxx>
Cc: "Masayuki Ohtake" <masa-korg@xxxxxxxxxxxxxxx>; "Sam Ravnborg" <sam@xxxxxxxxxxxx>; "Joe Perches" <joe@xxxxxxxxxxx>;
"LKML" <linux-kernel@xxxxxxxxxxxxxxx>; "ML netdev" <netdev@xxxxxxxxxxxxxxx>; "Greg Rose" <gregory.v.rose@xxxxxxxxx>;
"Maxime Bizon" <mbizon@xxxxxxxxxx>; "Kristoffer Glembo" <kristoffer@xxxxxxxxxxx>; "Ralf Baechle" <ralf@xxxxxxxxxxxxxx>;
"John Linn" <john.linn@xxxxxxxxxx>; "Randy Dunlap" <randy.dunlap@xxxxxxxxxx>; "David S. Miller" <davem@xxxxxxxxxxxxx>;
"MeeGo" <meego-dev@xxxxxxxxx>; "Wang, Qi" <qi.wang@xxxxxxxxx>; "Wang, Yong Y" <yong.y.wang@xxxxxxxxx>; "Andrew"
<andrew.chih.howe.khor@xxxxxxxxx>; "Intel OTC" <joel.clark@xxxxxxxxx>; "Foster, Margie" <margie.foster@xxxxxxxxx>;
"Toshiharu Okada" <okada533@xxxxxxxxxxxxxxx>; "Tomoya Morinaga" <morinaga526@xxxxxxxxxxxxxxx>; "Takahiro Shimizu"
<shimizu394@xxxxxxxxxxxxxxx>
Sent: Friday, September 03, 2010 12:10 AM
Subject: Re: [PATCH] Gigabit Ethernet driver of Topcliff PCH


On Thu, 02 Sep 2010 15:40:47 +0200
Eric Dumazet <eric.dumazet@xxxxxxxxx> wrote:

> Le jeudi 02 septembre 2010 à 21:39 +0900, Masayuki Ohtake a écrit :
> > Hi Eric
> >
> > Thank you for your comments.
> >
> > > I find hard to believe this driver needs to copy all outgoing frames on
> > > pre-allocated skbs.
> > >
> > > +       /* [Header:14][payload] ---> [Header:14][paddong:2][payload]    */
> > > +       memcpy(tmp_skb->data, skb->data, ETH_HLEN);
> > > +       tmp_skb->data[ETH_HLEN] = 0x00;
> > > +       tmp_skb->data[ETH_HLEN + 1] = 0x00;
> > > +       tmp_skb->len = skb->len;
> > > +       memcpy(&tmp_skb->data[ETH_HLEN + 2], &skb->data[ETH_HLEN],
> > > +              (skb->len - ETH_HLEN));
> > > +       buffer_info->kernel_skb = skb;
> > > +       skb = tmp_skb;
> > >
> > > Whats the deal here please ?
> >
> > This processing depends on hardware specification.
> >
> > At the time of transmission.
> > Hardware accepts a packet in the following format.
> >   [Header: 14octet] + [padding: 2octet] + [payload]
> >   Also, it is necessary to align the head of a [Header: 14octet] at 64byte.
> >
> > In my knowledge, SKB received by kernel are the following format.
> >   [padding: 2octet] + [Header:14octet] + [payload]
> >   Also, The head of [payload] has aligned at 16 byte.
> >
> > So, it has adjusted with the format of hardware by a copy.
>
> The two bytes padding can be handled by network stack, if you
> force in your setup phase :
> dev->hard_header_len = ETH_HLEN + 2;
>
> then, you can do a single memcpy:
>
> memcpy(tmp_skb->data, skb->data, skb->len);
>
> About the 64 byte alignement, it might be a bit more complex :)

With Eric's suggestion, you might find most packets are going
to be aligned but the code should not depend on it always being true.
Packets that get forwarded have header determined by the receiving
interface.

Something like:
       skb = skb_realloc_headroom(skb, 2);
       if (!skb)
           goto drop;
__skb_push(skb, 2);
        memmove(skb->data, skb->data + 2, ETH_HLEN);

skb->data[ETH_HLEN] = 0;
skb->data[ETH_HLEN+1] = 0;

        if (!IS_ALIGNED(skb->data, 16)) {
    nskb = netdev_alloc_skb(dev, skb->len + 16);
    if (!nskb)
                goto drop2;
            skb_reserve(nskb, PTR_ALIGN(skb->data, 16) - skb->data);
    skb_put(skb, skb->len);
            memcpy(nskb->data, skb->data, skb->len);
    dev_kfree_skb(skb);
            skb = nskb;
        }




-- 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/