netdev - Re: [WIP][PATCHES] Network xmit batching

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 03 Jul 2007 12:31:23 -0700
From:	"Matt Carlson" <mcarlson@...adcom.com>
To:	hadi@...erus.ca
cc:	"Robert Olsson" <Robert.Olsson@...a.slu.se>,
	"Evgeniy Polyakov" <johnpol@....mipt.ru>,
	"Krishna Kumar2" <krkumar2@...ibm.com>,
	"Gagan Arneja" <gaagaan@...il.com>, netdev@...r.kernel.org,
	"Rick Jones" <rick.jones2@...com>,
	"Sridhar Samudrala" <sri@...ibm.com>,
	"David Miller" <davem@...emloft.net>,
	"Jeff Garzik" <jeff@...zik.org>,
	"Michael Chan" <mchan@...adcom.com>
Subject: Re: [WIP][PATCHES] Network xmit batching - tg3 support

On Tue, 2007-07-03 at 09:09 -0400, jamal wrote:
> On Mon, 2007-02-07 at 14:20 -0700, Matt Carlson wrote:
> 
> 
> > 
> > Hi Jamal.  I'll be testing your patch soon,
> 
> much thanks. Please let me know if you need help while doing this.
> What tools are you planning to test with? I have tested this patch
> with pktgen on a dual opteron/tg3-buggy. 
> There is an outstanding issue in regards to clocksource - however the
> batch does well regardless of the clock source.

I had planned on using netperf, but pktgen sounds like a more controlled
environment.  Thanks for the tip.

> >  but I wanted to point out a
> > bug in the patch.  The patch defines TG3_SKB_CB() as follows :
> > 
> > #define TG3_SKB_CB(__skb) ((struct tg3_tx_cbdata *)&((__skb)->cb[0]))
> > 
> > This definition will collide with the VLAN macros if TG3_VLAN_TAG_USED
> > is set.  vlan_tx_tag_get() is defined as :
> > 
> > #define vlan_tx_tag_get(__skb)  (VLAN_TX_SKB_CB(__skb)->vlan_tag)
> > 
> > VLAN_TX_SKB_CB is defined as :
> > 
> > #define VLAN_TX_SKB_CB(__skb) \
> >         ((struct vlan_skb_tx_cookie *)&((__skb)->cb[0]))
> > 
> 
> yikes. Thanks for catching that - I thought i had this pretty much
> covered after scanning the source. So that bug exists on the e1000 as
> well.
> [It sounds very dangerous to me the way skb->cb is being used by the
> vlan code (i.e requires human intervention/knowledge to catch it as an
> issue). I had no freaking idea the vlan code was using it. Maybe a huge
> comment somewhere on how these cbs are being used by drivers would help
> or even a registration on startup to just make sure there is no conflict
> at a layer (i have been meaning to do the later for years now). In any
> case this is is a digression]. 
> 
> In the meantime, changing it to use byte 8 and above should do it? i.e.
> #define TG3_SKB_CB(__skb) ((struct tg3_tx_cbdata *)&((__skb)->cb[8]))
> 
> There are 48 bytes there on the skb-cb, so there should be plenty.

Do you see any reason why we couldn't just add the VLAN code to the prep
stage and simply overwrite that portion of the skb-cb?  Our driver would
just store its value in the base_flags member of tg3_tx_cbdata.

> > Also, I think the count, max_per_txd, and nr_frags fields of the
> > tg3_tx_cbdata struct are not needed.
> 
> Yes, you are right. That was a result of the LinuxWay(tm) (aka cutnpaste
> from the e1000 which needs them), Can you send me a patch for that and
> TG3_SKB_CB? 

Once we iron out the skb-cb issue, sure.

> 
> cheers,
> jamal
> 
> 

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