net: convert mvneta to build_skb()

From 0180a5e651dd771de18bf2c031ecfe7bb4c88d3e Mon Sep 17 00:00:00 2001

From 0180a5e651dd771de18bf2c031ecfe7bb4c88d3e Mon Sep 17 00:00:00 2001
From: Willy Tarreau <[email protected]>
Date: Sat, 15 Jun 2013 23:25:15 +0200
Subject: [PATCH] net: convert mvneta to build_skb()

We store the frag_size in the mvneta_port struct. In practice we'd need a single bit to know how to free the data, but since we need the size to call build_skb() anyway, let's store the full size.

With this patch, I observed a reproducible 2% performance improvement on HTTP-based benchmarks.

Signed-off-by Willy Tarreau <[email protected]>

From: Willy Tarreau <[email protected]>
Date: Thu, 4 Jul 2013 19:35:52 +0200

Quoted 12 lines

From 0180a5e651dd771de18bf2c031ecfe7bb4c88d3e Mon Sep 17 00:00:00 2001
From: Willy Tarreau <[email protected]>
Date: Sat, 15 Jun 2013 23:25:15 +0200
Subject: [PATCH] net: convert mvneta to build_skb()

We store the frag_size in the mvneta_port struct. In practice we'd need a single bit to know how to free the data, but since we need the size to call build_skb() anyway, let's store the full size.

With this patch, I observed a reproducible 2% performance improvement on HTTP-based benchmarks.

Quoted 1 line

Signed-off-by Willy Tarreau <[email protected]>

The net-next tree is closed now that the merge window is open, therefore submissions such as this are not appropriate at this time.

Please resubmit this when net-next opens again, an event which I will clearly announce here on netdev.

From 0180a5e651dd771de18bf2c031ecfe7bb4c88d3e Mon Sep 17 00:00:00 2001
From: Willy Tarreau <[email protected]>
Date: Sat, 15 Jun 2013 23:25:15 +0200
Subject: [PATCH] net: convert mvneta to build_skb()

We store the frag_size in the mvneta_port struct. In practice we'd need a single bit to know how to free the data, but since we need the size to call build_skb() anyway, let's store the full size.

With this patch, I observed a reproducible 2% performance improvement on HTTP-based benchmarks.

Signed-off-by Willy Tarreau <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Gregory CLEMENT <[email protected]>

Cc Eric Dumazet <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 71 ++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 21 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +0 -0

index c966785..0f2c6df 100644

Quoted 4 lines

drivers/net/ethernet/marvell/mvneta.c +0 -0

@@ -225,6 +225,7 @@ struct mvneta_stats {
 

drivers/net/ethernet/marvell/mvneta.c struct mvneta_stats { · line 231

Quoted 6 lines

drivers/net/ethernet/marvell/mvneta.c +1 -0

 struct mvneta_port {
 	int pkt_size;
+	unsigned int frag_size;
 	void __iomem *base;
 	struct mvneta_rx_queue *rxqs;
 	struct mvneta_tx_queue *txqs;

Thanks Willy. Sorry for asking such a stupid question, but I'm not very familiar with how this mechanism works. Can you explain why a single 'frag_size' field per port is sufficient? My concern is that this frag_size seems to be a per-packet information, but we have potentially multiple packets being received, and multiple RX queues. Is one single 'frag_size' per network interface sufficient?

For example, in mvneta_rx_refill(), you store the skb_size in pp->frag_size, and then you later re-use it in mvneta_rxq_drop_pkts. What guarantees you that mvneta_rx_refill() hasn't be called in the mean time for a different packet, in a different rxq, for the same network interface, and the value of pp->frag_size has been overridden?

Again, sorry for this stupid question, this is maybe just some basic networking knowledge that I'm missing here, but I thought it would be good to ask anyway.

Quoted 6 lines

Thanks Willy. Sorry for asking such a stupid question, but I'm not very familiar with how this mechanism works. Can you explain why a single 'frag_size' field per port is sufficient? My concern is that this frag_size seems to be a per-packet information, but we have potentially multiple packets being received, and multiple RX queues. Is one single 'frag_size' per network interface sufficient?

I had the exact same question when Eric sent me an experimental patch to do this on mv64xxx_eth a few months ago. Then I checked how the frag_size is computed. As you can see below, it does not depend on a per-packet size but on a per-port size which is in fact the MTU ("pkt_size" is the misleading part here) :

      skb_size = SKB_DATA_ALIGN(pp->pkt_size + MVNETA_MH_SIZE + NET_SKB_PAD) +
                 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));

So if skb_size depends solely on pp (struct mvneta_port), then it makes sense to have frag_size stored at the same place.

In practice we don't really need to store the frag_size in the struct, we just need to know if the data were allocated using netdev_alloc_frag() or kmalloc() to know how to free them. So a single bit would be enough, and I thought about just doing a +1 on the pointer when we need to free using kmalloc(). But that would add unneeded extra work in the fast path to fix the pointers. And since we need to pass frag_size to build_skb() it was a reasonable solution in my opinion.

Quoted 5 lines

For example, in mvneta_rx_refill(), you store the skb_size in pp->frag_size, and then you later re-use it in mvneta_rxq_drop_pkts. What guarantees you that mvneta_rx_refill() hasn't be called in the mean time for a different packet, in a different rxq, for the same network interface, and the value of pp->frag_size has been overridden?

It's not a problem since the refill() applies pp->pkt_size which doesn't change between calls. It's only changed in mvneta_change_mtu() which first stops the device. So I think it's safe.

Quoted 7 lines

In practice we don't really need to store the frag_size in the struct, we just need to know if the data were allocated using netdev_alloc_frag() or kmalloc() to know how to free them. So a single bit would be enough, and I thought about just doing a +1 on the pointer when we need to free using kmalloc(). But that would add unneeded extra work in the fast path to fix the pointers. And since we need to pass frag_size to build_skb() it was a reasonable solution in my opinion.

Aah, okay makes sense. So now, the question that comes up is why this skb_size calculation is done in every call of mvneta_rx_refill() ? It should be computed once, at the same time pkt_size is calculated, and stored in the mvneta_port structure? Then you just need to test whether it is smaller or larger than PAGE_SIZE to decide whether to use netdev_alloc_frag() vs. kmalloc().

I.e, I would turn all the:

	if (pp->frag_size)
		...
	else
		...

into:

	if (pp->skb_size <= PAGE_SIZE)
		...
	else
		...

Of course, you can always hide this test behind a small macro or inline function, to make it something like:

	if (mvneta_uses_small_skbs(pp))
		...
	else
		...

A better name than mvneta_uses_small_skbs() would of course be useful.

Quoted 6 lines

Quoted 5 lines

For example, in mvneta_rx_refill(), you store the skb_size in pp->frag_size, and then you later re-use it in mvneta_rxq_drop_pkts. What guarantees you that mvneta_rx_refill() hasn't be called in the mean time for a different packet, in a different rxq, for the same network interface, and the value of pp->frag_size has been overridden?

Quoted 3 lines

It's not a problem since the refill() applies pp->pkt_size which doesn't change between calls. It's only changed in mvneta_change_mtu() which first stops the device. So I think it's safe.

Yeah, sure, now that I see that pp->frag_size is constant for a given MTU configuration, it looks safe. But I find the current code that retests and reassigns pp->frag_size at every call of rxq_refill() to be very confusing.

From 0180a5e651dd771de18bf2c031ecfe7bb4c88d3e Mon Sep 17 00:00:00 2001
From: Willy Tarreau <[email protected]>
Date: Sat, 15 Jun 2013 23:25:15 +0200
Subject: [PATCH] net: convert mvneta to build_skb()

We store the frag_size in the mvneta_port struct. In practice we'd need a single bit to know how to free the data, but since we need the size to call build_skb() anyway, let's store the full size.

With this patch, I observed a reproducible 2% performance improvement on HTTP-based benchmarks.

Signed-off-by Willy Tarreau <[email protected]>

Dear Willy Tarreau,

Quoted 9 lines · Willy Tarreau

From 0180a5e651dd771de18bf2c031ecfe7bb4c88d3e Mon Sep 17 00:00:00 2001
From: Willy Tarreau <[email protected]>
Date: Sat, 15 Jun 2013 23:25:15 +0200
Subject: [PATCH] net: convert mvneta to build_skb()

We store the frag_size in the mvneta_port struct. In practice we'd need a single bit to know how to free the data, but since we need the size to call build_skb() anyway, let's store the full size.

Quoted 4 lines

With this patch, I observed a reproducible 2% performance improvement on HTTP-based benchmarks.

Signed-off-by Willy Tarreau <[email protected]>

3.11-rc1 is now out. Do you intend to resend an updated version of your patches, according to the discussions we had?

Quoted 2 lines

3.11-rc1 is now out. Do you intend to resend an updated version of your patches, according to the discussions we had?

Yes I'll do. This week-end I have tried to pre-compute the frag_size. The performance did not change, nor the driver size, but the code is cleaner and more readable (in my opinion).

I have noticed your recent patches to support big-endian, and since we risk to get conflicts by touching similar areas, I guess you'll prefer to have a clean patch series that applies well.

I'm still checking a little bit what can be easily improved (if any) and will resend.

The big-endian support is pretty light in terms of driver changes, so if there are conflicts, they should be easy to solve.

Quoted 2 lines · Thomas Petazzoni

The big-endian support is pretty light in terms of driver changes, so if there are conflicts, they should be easy to solve.

I agree.

BTW, I'm still having the patch to re-enable phy-polling at boot in my queue. Last time I tried without it I still only had eth0 working. I completely forgot about this one. Since I did it blindly, I have no way to verify if what I've done is necessary according to the specs. But I'm quite sure that the NICs that are not enabled by the boot loaders don't work.

If you want, I'll resend it to you at the same time.

Dear Willy Tarreau,

Quoted 13 lines · Willy Tarreau

Quoted 2 lines · Thomas Petazzoni

The big-endian support is pretty light in terms of driver changes, so if there are conflicts, they should be easy to solve.

I agree.

BTW, I'm still having the patch to re-enable phy-polling at boot in my queue. Last time I tried without it I still only had eth0 working. I completely forgot about this one. Since I did it blindly, I have no way to verify if what I've done is necessary according to the specs. But I'm quite sure that the NICs that are not enabled by the boot loaders don't work.

Quoted 1 line

If you want, I'll resend it to you at the same time.

I still believe this is not the right fix, but unfortunately, I haven't had the time to work on what would be the right fix. Hardware PHY polling has been intentionally disabled, and the driver should work without, since the PHY polling is done in software by the phylib.

Quoted 4 lines

I still believe this is not the right fix, but unfortunately, I haven't had the time to work on what would be the right fix. Hardware PHY polling has been intentionally disabled, and the driver should work without, since the PHY polling is done in software by the phylib.

You may now be able to use the PHY interrupt bit from your MAC interrupt handler and call phy_mac_interrupt() provided by libphy. I added it specifically to be able to use PHY link interrupts connected to the MAC.

Quoted 4 lines

I still believe this is not the right fix, but unfortunately, I haven't had the time to work on what would be the right fix. Hardware PHY polling has been intentionally disabled, and the driver should work without, since the PHY polling is done in software by the phylib.

You may now be able to use the PHY interrupt bit from your MAC interrupt handler and call phy_mac_interrupt() provided by libphy. I added it specifically to be able to use PHY link interrupts connected to the MAC.

Dear Florian Fainelli,

Quoted 5 lines · Florian Fainelli

Quoted 4 lines

I still believe this is not the right fix, but unfortunately, I haven't had the time to work on what would be the right fix. Hardware PHY polling has been intentionally disabled, and the driver should work without, since the PHY polling is done in software by the phylib.

Quoted 4 lines

You may now be able to use the PHY interrupt bit from your MAC interrupt handler and call phy_mac_interrupt() provided by libphy. I added it specifically to be able to use PHY link interrupts connected to the MAC.

Right, but the MAC feature in question makes the MAC directly talks to the PHY to configure it, completely bypassing the entire libphy logic. I am not sure how to handle this together with libphy.