Why bother with last_rx at all on loopback. I have been thinking
we should figure out a way to get rid of last_rx all together. It only
seems to be used by bonding, and the bonding driver could do the calculation
in its receive handling.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Re: [tbench regression fixes]: digging out smelly deadmen.

On Fri, Oct 31, 2008 at 12:57:13PM -0700, Stephen Hemminger (shemminger@vyatta.com) wrote:
> Why bother with last_rx at all on loopback. I have been thinking
> we should figure out a way to get rid of last_rx all together. It only
> seems to be used by bonding, and the bonding driver could do the calculation
> in its receive handling.

Not related to the regression: bug will be just papered out by this
changes. Having bonding on loopback is somewhat strange idea, but still
this kind of changes is an attempt to make a good play in the bad game:
this loopback-only optimization does not fix the problem.

Re: [tbench regression fixes]: digging out smelly deadmen.

Evgeniy Polyakov a écrit :
> On Fri, Oct 31, 2008 at 12:57:13PM -0700, Stephen Hemminger (shemminger@vyatta.com) wrote:
>> Why bother with last_rx at all on loopback. I have been thinking
>> we should figure out a way to get rid of last_rx all together. It only
>> seems to be used by bonding, and the bonding driver could do the calculation
>> in its receive handling.
>
> Not related to the regression: bug will be just papered out by this
> changes. Having bonding on loopback is somewhat strange idea, but still
> this kind of changes is an attempt to make a good play in the bad game:
> this loopback-only optimization does not fix the problem.
>

Just to be clear, this change was not meant to be committed.
It already was rejected by David some years ago (2005, and 2006)

Re: [tbench regression fixes]: digging out smelly deadmen.

Hi Eric.

On Fri, Oct 31, 2008 at 10:03:00PM +0100, Eric Dumazet (dada1@cosmosbay.com) wrote:
> Just to be clear, this change was not meant to be committed.
> It already was rejected by David some years ago (2005, and 2006)
>
> http://www.mail-archive.com/netdev@v.../msg07382.html
>
> If you read my mail, I was *only* saying that tbench results can be
> sensible to
> cache line ping pongs. tbench is a crazy benchmark, and only is a crazy
> benchmark.

No problem Eric, I just pointed that this particular case is rather
fluffy, which really does not fix anything. It improves the case, but
the way it does it, is not the right one imho.
We would definitely want to eliminate assignment of global constantly
updated variables in the pathes where it is not required, but in a
way which does improve the design and implementation, but not to
hide some other problem.

Tbench is, well, as is it is quite usual network server
Dbench side is rather non-optimized, but still it is quite common
pattern of small-sized IO. Anyway, optimizing for some kind of the
workload tends to force other side to become slower, so I agree of
course that any narrow-viewed optimizations are bad, and instead we
should focus on searching error patter more widerspread.

Since bonding driver doesn't actually see the rx packets, that isn't
really possible. But it would be possible to change last_rx from a variable
to an function pointer, so that device's could apply other logic to derive
the last value. One example would be to keep it per cpu and then take the
maximum.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Re: [tbench regression fixes]: digging out smelly deadmen.

From: Eric Dumazet
Date: Fri, 31 Oct 2008 22:03:00 +0100
> Evgeniy Polyakov a écrit :
> > On Fri, Oct 31, 2008 at 12:57:13PM -0700, Stephen Hemminger (shemminger@vyatta.com) wrote:
> >> Why bother with last_rx at all on loopback. I have been thinking
> >> we should figure out a way to get rid of last_rx all together. It only
> >> seems to be used by bonding, and the bonding driver could do the calculation
> >> in its receive handling.
> > Not related to the regression: bug will be just papered out by this
> > changes. Having bonding on loopback is somewhat strange idea, but still
> > this kind of changes is an attempt to make a good play in the bad game:
> > this loopback-only optimization does not fix the problem.
>
> Just to be clear, this change was not meant to be committed.
> It already was rejected by David some years ago (2005, and 2006)
>
> http://www.mail-archive.com/netdev@v.../msg07382.html

However, I do like Stephen's suggestion that maybe we can get rid of
this ->last_rx thing by encapsulating the logic completely in the
bonding driver.
> If you read my mail, I was *only* saying that tbench results can be sensible to
> cache line ping pongs. tbench is a crazy benchmark, and only is a crazy benchmark.
>
> Optimizing linux for tbench sake would be .... crazy ?

Unlike dbench I think tbench is worth cranking up as much as possible.

It doesn't have a huge memory working set, it just writes mostly small
messages over a TCP socket back and forth, and does a lot of blocking

And I think we'd like all of those operating to run as fast as possible.

When Tridge first wrote tbench I would see the expected things at the
top of the profiles. Things like tcp_ack(), copy to/from user, and
perhaps SLAB.

Re: [tbench regression fixes]: digging out smelly deadmen.

Stephen Hemminger wrote:
>On Fri, 31 Oct 2008 16:51:44 -0700 (PDT)
>David Miller wrote:
[...]
>> However, I do like Stephen's suggestion that maybe we can get rid of
>> this ->last_rx thing by encapsulating the logic completely in the
>> bonding driver.
>
>Since bonding driver doesn't actually see the rx packets, that isn't
>really possible. But it would be possible to change last_rx from a variable
>to an function pointer, so that device's could apply other logic to derive
>the last value. One example would be to keep it per cpu and then take the
>maximum.

I suspect it could also be tucked away in skb_bond_should_drop,
which is called both by the standard input path and the VLAN accelerated
path to see if the packet should be tossed (e.g., it arrived on an
inactive bonding slave).

Since last_rx is part of struct net_device, I don't think any
additional bonding internals knowledge would be needed. It could be
arranged to only update last_rx for devices that are actually bonding
slaves.

Just off the top of my head (haven't tested this), something
like this:

That doesn't move the storage out of struct net_device, but it
does stop the updates for devices that aren't bonding slaves. It could
probably be refined further to only update when the ARP monitor is
running (the gizmo that uses last_rx).

Re: [tbench regression fixes]: digging out smelly deadmen.

From: Jay Vosburgh
Date: Fri, 31 Oct 2008 17:16:33 -0700
> I suspect it could also be tucked away in skb_bond_should_drop,
> which is called both by the standard input path and the VLAN accelerated
> path to see if the packet should be tossed (e.g., it arrived on an
> inactive bonding slave).
>
> Since last_rx is part of struct net_device, I don't think any
> additional bonding internals knowledge would be needed. It could be
> arranged to only update last_rx for devices that are actually bonding
> slaves.
>
> Just off the top of my head (haven't tested this), something
> like this:
...
>
> That doesn't move the storage out of struct net_device, but it
> does stop the updates for devices that aren't bonding slaves. It could
> probably be refined further to only update when the ARP monitor is
> running (the gizmo that uses last_rx).

I like this very much.

Jay can you give this a quick test by just trying this patch
and removing the ->last_rx setting in the driver you use for
your test?

Once you do that, I'll apply this to net-next-2.6 and do the
leg work to zap all of the ->last_rx updates from the entire tree.

The only user of the net_device->last_rx field is bonding. This
patch adds a conditional update of last_rx to the bonding special logic
in skb_bond_should_drop, causing last_rx to only be updated when the ARP
monitor is running.

This frees network device drivers from the necessity of updating
last_rx, which can have cache line thrash issues.

From: Jay Vosburgh
Date: Mon, 03 Nov 2008 18:13:09 -0800
>
> The only user of the net_device->last_rx field is bonding. This
> patch adds a conditional update of last_rx to the bonding special logic
> in skb_bond_should_drop, causing last_rx to only be updated when the ARP
> monitor is running.
>
> This frees network device drivers from the necessity of updating
> last_rx, which can have cache line thrash issues.
>
> Signed-off-by: Jay Vosburgh