Netdev List
 help / color / mirror / Atom feed
* [PATCH 3/3] powerpc: bpf: implement in-register swap for 64-bit endian operations
From: Naveen N. Rao @ 2017-01-13 17:10 UTC (permalink / raw)
  To: mpe; +Cc: linuxppc-dev, netdev, ast, daniel, davem
In-Reply-To: <e73efe6facf6c06932b4a87707e5978172ee773e.1484326337.git.naveen.n.rao@linux.vnet.ibm.com>

Generate instructions to perform the endian conversion using registers,
rather than generating two memory accesses.

The "way easier and faster" comment was obviously for the author, not
the processor.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/net/bpf_jit_comp64.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
index 1e313db..0413a89 100644
--- a/arch/powerpc/net/bpf_jit_comp64.c
+++ b/arch/powerpc/net/bpf_jit_comp64.c
@@ -599,16 +599,22 @@ static int bpf_jit_build_body(struct bpf_prog *fp, u32 *image,
 				break;
 			case 64:
 				/*
-				 * Way easier and faster(?) to store the value
-				 * into stack and then use ldbrx
+				 * We'll split it up into two words, swap those
+				 * independently and then merge them back.
 				 *
-				 * ctx->seen will be reliable in pass2, but
-				 * the instructions generated will remain the
-				 * same across all passes
+				 * First up, let's swap the most-significant word.
 				 */
-				PPC_STD(dst_reg, 1, bpf_jit_stack_local(ctx));
-				PPC_ADDI(b2p[TMP_REG_1], 1, bpf_jit_stack_local(ctx));
-				PPC_LDBRX(dst_reg, 0, b2p[TMP_REG_1]);
+				PPC_RLDICL(b2p[TMP_REG_1], dst_reg, 32, 32);
+				PPC_RLWINM(b2p[TMP_REG_2], b2p[TMP_REG_1], 8, 0, 31);
+				PPC_RLWIMI(b2p[TMP_REG_2], b2p[TMP_REG_1], 24, 0, 7);
+				PPC_RLWIMI(b2p[TMP_REG_2], b2p[TMP_REG_1], 24, 16, 23);
+				/* Then, the second half */
+				PPC_RLWINM(b2p[TMP_REG_1], dst_reg, 8, 0, 31);
+				PPC_RLWIMI(b2p[TMP_REG_1], dst_reg, 24, 0, 7);
+				PPC_RLWIMI(b2p[TMP_REG_1], dst_reg, 24, 16, 23);
+				/* Merge back */
+				PPC_RLDICR(dst_reg, b2p[TMP_REG_1], 32, 31);
+				PPC_OR(dst_reg, dst_reg, b2p[TMP_REG_2]);
 				break;
 			}
 			break;
-- 
2.10.2

^ permalink raw reply related

* [PATCH net-next] netlink: do not enter direct reclaim from netlink_trim()
From: Eric Dumazet @ 2017-01-13 17:11 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

From: Eric Dumazet <edumazet@google.com>

In commit d35c99ff77ecb ("netlink: do not enter direct reclaim from
netlink_dump()") we made sure to not trigger expensive memory reclaim.

Problem is that a bit later, netlink_trim() might be called and
trigger memory reclaim.

netlink_trim() should be best effort, and really as fast as possible.
Under memory pressure, it is fine to not trim this skb.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/netlink/af_netlink.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 161b628ab2b08bf4321dbe617022c4c50486534d..edcc1e19ad532641f51f6809b8c90d1e377081ff 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1210,7 +1210,9 @@ static struct sk_buff *netlink_trim(struct sk_buff *skb, gfp_t allocation)
 		skb = nskb;
 	}
 
-	if (!pskb_expand_head(skb, 0, -delta, allocation))
+	if (!pskb_expand_head(skb, 0, -delta,
+			      (allocation & ~__GFP_DIRECT_RECLAIM) |
+			      __GFP_NOWARN | __GFP_NORETRY))
 		skb->truesize -= delta;
 
 	return skb;

^ permalink raw reply related

* Re: kill off pci_enable_msi_{exact,range}
From: Bjorn Helgaas @ 2017-01-13 17:13 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-pci, Mauro Carvalho Chehab, netdev, linux-media,
	Tom Lendacky
In-Reply-To: <20170113080553.GA26280@lst.de>

On Fri, Jan 13, 2017 at 09:05:53AM +0100, Christoph Hellwig wrote:
> On Fri, Jan 13, 2017 at 08:55:03AM +0100, Christoph Hellwig wrote:
> > On Thu, Jan 12, 2017 at 03:29:00PM -0600, Bjorn Helgaas wrote:
> > > Applied all three (with Tom's ack on the amd-xgbe patch) to pci/msi for
> > > v4.11, thanks!
> > 
> > Tom had just send me an event better version of the xgbe patch.  Tom,
> > maybe you can resend that relative to the PCI tree [1], so that we don't
> > lose it for next merge window?
> 
> Actually - Bjorn, your msi branch contains an empty commit from this
> thread:
> 
> 	https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/msi&id=7a8191de43faa9869b421a1b06075d8126ce7c0b

Yep, I botched that.  Thought I'd fixed it, but guess I got distracted.

> Maybe we should rebase it after all to avoid that?  In that case please
> pick up the xgbe patch from Tom below:

I dropped the empty commit and replaced the xgbe patch with the one below.
Can you take a look at [1] and make sure it's what you expected?

[1] https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/msi

Thanks!

> ---
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Subject: [PATCH] amd-xgbe: Update PCI support to use new IRQ functions
> 
> Some of the PCI MSI/MSI-X functions have been deprecated and it is
> recommended to use the new pci_alloc_irq_vectors() function. Convert
> the code over to use the new function. Also, modify the way in which
> the IRQs are requested - try for multiple MSI-X/MSI first, then a
> single MSI/legacy interrupt.
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/net/ethernet/amd/xgbe/xgbe-pci.c |  128 +++++++++---------------------
>  drivers/net/ethernet/amd/xgbe/xgbe.h     |    8 +-
>  2 files changed, 41 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
> index e76b7f6..e436902 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-pci.c
> @@ -122,104 +122,40 @@
>  #include "xgbe.h"
>  #include "xgbe-common.h"
>  
> -static int xgbe_config_msi(struct xgbe_prv_data *pdata)
> +static int xgbe_config_multi_msi(struct xgbe_prv_data *pdata)
>  {
> -	unsigned int msi_count;
> +	unsigned int vector_count;
>  	unsigned int i, j;
>  	int ret;
>  
> -	msi_count = XGBE_MSIX_BASE_COUNT;
> -	msi_count += max(pdata->rx_ring_count,
> -			 pdata->tx_ring_count);
> -	msi_count = roundup_pow_of_two(msi_count);
> +	vector_count = XGBE_MSI_BASE_COUNT;
> +	vector_count += max(pdata->rx_ring_count,
> +			    pdata->tx_ring_count);
>  
> -	ret = pci_enable_msi_exact(pdata->pcidev, msi_count);
> +	ret = pci_alloc_irq_vectors(pdata->pcidev, XGBE_MSI_MIN_COUNT,
> +				    vector_count, PCI_IRQ_MSI | PCI_IRQ_MSIX);
>  	if (ret < 0) {
> -		dev_info(pdata->dev, "MSI request for %u interrupts failed\n",
> -			 msi_count);
> -
> -		ret = pci_enable_msi(pdata->pcidev);
> -		if (ret < 0) {
> -			dev_info(pdata->dev, "MSI enablement failed\n");
> -			return ret;
> -		}
> -
> -		msi_count = 1;
> -	}
> -
> -	pdata->irq_count = msi_count;
> -
> -	pdata->dev_irq = pdata->pcidev->irq;
> -
> -	if (msi_count > 1) {
> -		pdata->ecc_irq = pdata->pcidev->irq + 1;
> -		pdata->i2c_irq = pdata->pcidev->irq + 2;
> -		pdata->an_irq = pdata->pcidev->irq + 3;
> -
> -		for (i = XGBE_MSIX_BASE_COUNT, j = 0;
> -		     (i < msi_count) && (j < XGBE_MAX_DMA_CHANNELS);
> -		     i++, j++)
> -			pdata->channel_irq[j] = pdata->pcidev->irq + i;
> -		pdata->channel_irq_count = j;
> -
> -		pdata->per_channel_irq = 1;
> -		pdata->channel_irq_mode = XGBE_IRQ_MODE_LEVEL;
> -	} else {
> -		pdata->ecc_irq = pdata->pcidev->irq;
> -		pdata->i2c_irq = pdata->pcidev->irq;
> -		pdata->an_irq = pdata->pcidev->irq;
> -	}
> -
> -	if (netif_msg_probe(pdata))
> -		dev_dbg(pdata->dev, "MSI interrupts enabled\n");
> -
> -	return 0;
> -}
> -
> -static int xgbe_config_msix(struct xgbe_prv_data *pdata)
> -{
> -	unsigned int msix_count;
> -	unsigned int i, j;
> -	int ret;
> -
> -	msix_count = XGBE_MSIX_BASE_COUNT;
> -	msix_count += max(pdata->rx_ring_count,
> -			  pdata->tx_ring_count);
> -
> -	pdata->msix_entries = devm_kcalloc(pdata->dev, msix_count,
> -					   sizeof(struct msix_entry),
> -					   GFP_KERNEL);
> -	if (!pdata->msix_entries)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < msix_count; i++)
> -		pdata->msix_entries[i].entry = i;
> -
> -	ret = pci_enable_msix_range(pdata->pcidev, pdata->msix_entries,
> -				    XGBE_MSIX_MIN_COUNT, msix_count);
> -	if (ret < 0) {
> -		dev_info(pdata->dev, "MSI-X enablement failed\n");
> -		devm_kfree(pdata->dev, pdata->msix_entries);
> -		pdata->msix_entries = NULL;
> +		dev_info(pdata->dev, "multi MSI/MSI-X enablement failed\n");
>  		return ret;
>  	}
>  
>  	pdata->irq_count = ret;
>  
> -	pdata->dev_irq = pdata->msix_entries[0].vector;
> -	pdata->ecc_irq = pdata->msix_entries[1].vector;
> -	pdata->i2c_irq = pdata->msix_entries[2].vector;
> -	pdata->an_irq = pdata->msix_entries[3].vector;
> +	pdata->dev_irq = pci_irq_vector(pdata->pcidev, 0);
> +	pdata->ecc_irq = pci_irq_vector(pdata->pcidev, 1);
> +	pdata->i2c_irq = pci_irq_vector(pdata->pcidev, 2);
> +	pdata->an_irq = pci_irq_vector(pdata->pcidev, 3);
>  
> -	for (i = XGBE_MSIX_BASE_COUNT, j = 0; i < ret; i++, j++)
> -		pdata->channel_irq[j] = pdata->msix_entries[i].vector;
> +	for (i = XGBE_MSI_BASE_COUNT, j = 0; i < ret; i++, j++)
> +		pdata->channel_irq[j] = pci_irq_vector(pdata->pcidev, i);
>  	pdata->channel_irq_count = j;
>  
>  	pdata->per_channel_irq = 1;
>  	pdata->channel_irq_mode = XGBE_IRQ_MODE_LEVEL;
>  
>  	if (netif_msg_probe(pdata))
> -		dev_dbg(pdata->dev, "MSI-X interrupts enabled\n");
> +		dev_dbg(pdata->dev, "multi %s interrupts enabled\n",
> +			pdata->pcidev->msix_enabled ? "MSI-X" : "MSI");
>  
>  	return 0;
>  }
> @@ -228,21 +164,28 @@ static int xgbe_config_irqs(struct xgbe_prv_data *pdata)
>  {
>  	int ret;
>  
> -	ret = xgbe_config_msix(pdata);
> +	ret = xgbe_config_multi_msi(pdata);
>  	if (!ret)
>  		goto out;
>  
> -	ret = xgbe_config_msi(pdata);
> -	if (!ret)
> -		goto out;
> +	ret = pci_alloc_irq_vectors(pdata->pcidev, 1, 1,
> +				    PCI_IRQ_LEGACY | PCI_IRQ_MSI);
> +	if (ret < 0) {
> +		dev_info(pdata->dev, "single IRQ enablement failed\n");
> +		return ret;
> +	}
>  
>  	pdata->irq_count = 1;
> -	pdata->irq_shared = 1;
> +	pdata->channel_irq_count = 1;
> +
> +	pdata->dev_irq = pci_irq_vector(pdata->pcidev, 0);
> +	pdata->ecc_irq = pci_irq_vector(pdata->pcidev, 0);
> +	pdata->i2c_irq = pci_irq_vector(pdata->pcidev, 0);
> +	pdata->an_irq = pci_irq_vector(pdata->pcidev, 0);
>  
> -	pdata->dev_irq = pdata->pcidev->irq;
> -	pdata->ecc_irq = pdata->pcidev->irq;
> -	pdata->i2c_irq = pdata->pcidev->irq;
> -	pdata->an_irq = pdata->pcidev->irq;
> +	if (netif_msg_probe(pdata))
> +		dev_dbg(pdata->dev, "single %s interrupt enabled\n",
> +			pdata->pcidev->msi_enabled ?  "MSI" : "legacy");
>  
>  out:
>  	if (netif_msg_probe(pdata)) {
> @@ -412,12 +355,15 @@ static int xgbe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	/* Configure the netdev resource */
>  	ret = xgbe_config_netdev(pdata);
>  	if (ret)
> -		goto err_pci_enable;
> +		goto err_irq_vectors;
>  
>  	netdev_notice(pdata->netdev, "net device enabled\n");
>  
>  	return 0;
>  
> +err_irq_vectors:
> +	pci_free_irq_vectors(pdata->pcidev);
> +
>  err_pci_enable:
>  	xgbe_free_pdata(pdata);
>  
> @@ -433,6 +379,8 @@ static void xgbe_pci_remove(struct pci_dev *pdev)
>  
>  	xgbe_deconfig_netdev(pdata);
>  
> +	pci_free_irq_vectors(pdata->pcidev);
> +
>  	xgbe_free_pdata(pdata);
>  }
>  
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
> index f52a9bd..99f1c87 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe.h
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
> @@ -211,9 +211,9 @@
>  #define XGBE_MAC_PROP_OFFSET	0x1d000
>  #define XGBE_I2C_CTRL_OFFSET	0x1e000
>  
> -/* PCI MSIx support */
> -#define XGBE_MSIX_BASE_COUNT	4
> -#define XGBE_MSIX_MIN_COUNT	(XGBE_MSIX_BASE_COUNT + 1)
> +/* PCI MSI/MSIx support */
> +#define XGBE_MSI_BASE_COUNT	4
> +#define XGBE_MSI_MIN_COUNT	(XGBE_MSI_BASE_COUNT + 1)
>  
>  /* PCI clock frequencies */
>  #define XGBE_V2_DMA_CLOCK_FREQ	500000000	/* 500 MHz */
> @@ -980,14 +980,12 @@ struct xgbe_prv_data {
>  	unsigned int desc_ded_count;
>  	unsigned int desc_sec_count;
>  
> -	struct msix_entry *msix_entries;
>  	int dev_irq;
>  	int ecc_irq;
>  	int i2c_irq;
>  	int channel_irq[XGBE_MAX_DMA_CHANNELS];
>  
>  	unsigned int per_channel_irq;
> -	unsigned int irq_shared;
>  	unsigned int irq_count;
>  	unsigned int channel_irq_count;
>  	unsigned int channel_irq_mode;
> 

^ permalink raw reply

* Re: [pull request][for-next] Mellanox mlx5 Reorganize core driver directory layout
From: David Miller @ 2017-01-13 17:14 UTC (permalink / raw)
  To: saeedm-VPRAkNaXOzVWk0Htik3J/w
  Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, leon-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <1484241755-17603-1-git-send-email-saeedm-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

From: Saeed Mahameed <saeedm-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Date: Thu, 12 Jan 2017 19:22:34 +0200

> This pull request includes one patch from Leon, this patch as described 
> below will change the driver directory structure and layout for better,
> logical and modular driver files separation.
> 
> This change is important to both rdma and net maintainers in order to 
> have smoother management of driver patches for different mlx5 sub modules
> and smoother rdma-next vs. net-next features submissions.
> 
> Please find more info below -in the tag commit message-,
> review and let us know if there's any problem.
> 
> This change doesn't introduce any conflicts with the current mlx5
> fixes and cleanups posted on 2017-01-10 to net branch, and merge tests
> worked flawlessly with no issues.
> 
> This is the last pull request meant for both rdma-next and net-next.
> Once pulled, this will be the base shared code for both trees.

This is pretty crazy, it will make all bug fix backporting to -stable
a complete nightmare for myself, Doug, various distribution maintainers
and many other people who quietly have to maintain their own trees and
do backporting.

I really don't think you can justify this rearrangement based upon the
consequences and how much activity happens in this driver.

You should have thought long and hard about the layout a long time ago
rather than after the driver has been in the tree for many years.

Sorry.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: kill off pci_enable_msi_{exact,range}
From: Christoph Hellwig @ 2017-01-13 17:15 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Christoph Hellwig, linux-pci, Mauro Carvalho Chehab, netdev,
	linux-media, Tom Lendacky
In-Reply-To: <20170113171321.GA22776@bhelgaas-glaptop.roam.corp.google.com>

On Fri, Jan 13, 2017 at 11:13:21AM -0600, Bjorn Helgaas wrote:
> I dropped the empty commit and replaced the xgbe patch with the one below.
> Can you take a look at [1] and make sure it's what you expected?

This looks great, thanks!

^ permalink raw reply

* Re: resend: tcp: performance issue with fastopen connections (mss > window)
From: Eric Dumazet @ 2017-01-13 17:14 UTC (permalink / raw)
  To: Alexey Kodanev
  Cc: Eric Dumazet, David Miller, netdev, Vasily Isaenko, Neal Cardwell,
	Yuchung Cheng
In-Reply-To: <22b07900-2151-a31f-34aa-7fb47c958423@oracle.com>

On Fri, Jan 13, 2017 at 9:07 AM, Alexey Kodanev
<alexey.kodanev@oracle.com> wrote:
> Hi Eric,
> On 13.01.2017 18:35, Eric Dumazet wrote:

>> Care to send a packetdrill test so that we have a clear picture of what
>> is going on ?
>
> Is it capable of making two connections in the single test, one after
> another?

Absolutely.

Neal, Yuchung would you be kind enough to send a Fastopen tpacketdrill
template showing a typical fastopen test
running on an upstream kernel ?

Thanks !

^ permalink raw reply

* Re: [PATCH v2 0/2] bpf: add longest prefix match map
From: David Miller @ 2017-01-13 17:15 UTC (permalink / raw)
  To: daniel; +Cc: ast, dh.herrmann, daniel, netdev
In-Reply-To: <20170112172922.16636-1-daniel@zonque.org>

From: Daniel Mack <daniel@zonque.org>
Date: Thu, 12 Jan 2017 18:29:20 +0100

> Feedback is much appreciated.

Daniel and Alexei, can I get a review please?

^ permalink raw reply

* RE: [PATCH 3/3] powerpc: bpf: implement in-register swap for 64-bit endian operations
From: David Laight @ 2017-01-13 17:17 UTC (permalink / raw)
  To: 'Naveen N. Rao', mpe@ellerman.id.au
  Cc: linuxppc-dev@lists.ozlabs.org, netdev@vger.kernel.org, ast@fb.com,
	daniel@iogearbox.net, davem@davemloft.net
In-Reply-To: <bb264395301754f43b77ddec68a16dd34220abb4.1484326337.git.naveen.n.rao@linux.vnet.ibm.com>

From: Naveen N. Rao
> Sent: 13 January 2017 17:10
> Generate instructions to perform the endian conversion using registers,
> rather than generating two memory accesses.
> 
> The "way easier and faster" comment was obviously for the author, not
> the processor.

That rather depends on whether the processor has a store to load forwarder
that will satisfy the read from the store buffer.
I don't know about ppc, but at least some x86 will do that.

	David

^ permalink raw reply

* Re: [PATCH net] net: systemport: Decouple flow control from __bcm_sysport_tx_reclaim
From: David Miller @ 2017-01-13 17:20 UTC (permalink / raw)
  To: f.fainelli; +Cc: netdev
In-Reply-To: <20170112200909.26825-1-f.fainelli@gmail.com>

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Thu, 12 Jan 2017 12:09:09 -0800

> The __bcm_sysport_tx_reclaim() function is used to reclaim transmit
> resources in different places within the driver. Most of them should
> not affect the state of the transit flow control.
> 
> Introduce bcm_sysport_tx_clean() which cleans the ring, but does not
> re-enable flow control towards the networking stack, and make
> bcm_sysport_tx_reclaim() do the actual transmit queue flow control.
> 
> Fixes: 80105befdb4b ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Applied and queued up for -stable.

^ permalink raw reply

* Re: [PATCH net-next] netlink: do not enter direct reclaim from netlink_trim()
From: Alexei Starovoitov @ 2017-01-13 17:20 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <1484327482.13165.38.camel@edumazet-glaptop3.roam.corp.google.com>

On Fri, Jan 13, 2017 at 09:11:22AM -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> In commit d35c99ff77ecb ("netlink: do not enter direct reclaim from
> netlink_dump()") we made sure to not trigger expensive memory reclaim.
> 
> Problem is that a bit later, netlink_trim() might be called and
> trigger memory reclaim.
> 
> netlink_trim() should be best effort, and really as fast as possible.
> Under memory pressure, it is fine to not trim this skb.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Thanks!
Acked-by: Alexei Starovoitov <ast@kernel.org>

^ permalink raw reply

* Re: [net PATCH 5/5] virtio_net: XDP support for adjust_head
From: Michael S. Tsirkin @ 2017-01-13 17:23 UTC (permalink / raw)
  To: John Fastabend
  Cc: jasowang, john.r.fastabend, netdev, alexei.starovoitov, daniel
In-Reply-To: <20170112214519.3039.27879.stgit@john-Precision-Tower-5810>

On Thu, Jan 12, 2017 at 01:45:19PM -0800, John Fastabend wrote:
> Add support for XDP adjust head by allocating a 256B header region
> that XDP programs can grow into. This is only enabled when a XDP
> program is loaded.
> 
> In order to ensure that we do not have to unwind queue headroom push
> queue setup below bpf_prog_add. It reads better to do a prog ref
> unwind vs another queue setup call.
> 
> At the moment this code must do a full reset to ensure old buffers
> without headroom on program add or with headroom on program removal
> are not used incorrectly in the datapath. Ideally we would only
> have to disable/enable the RX queues being updated but there is no
> API to do this at the moment in virtio so use the big hammer. In
> practice it is likely not that big of a problem as this will only
> happen when XDP is enabled/disabled changing programs does not
> require the reset. There is some risk that the driver may either
> have an allocation failure or for some reason fail to correctly
> negotiate with the underlying backend in this case the driver will
> be left uninitialized. I have not seen this ever happen on my test
> systems and for what its worth this same failure case can occur
> from probe and other contexts in virtio framework.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
>  drivers/net/virtio_net.c |  155 ++++++++++++++++++++++++++++++++++++++++------
>  drivers/virtio/virtio.c  |    9 ++-
>  include/linux/virtio.h   |    3 +
>  3 files changed, 144 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6041828..8b897e7 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -28,6 +28,7 @@
>  #include <linux/slab.h>
>  #include <linux/cpu.h>
>  #include <linux/average.h>
> +#include <linux/pci.h>
>  #include <net/busy_poll.h>
>  
>  static int napi_weight = NAPI_POLL_WEIGHT;
> @@ -159,6 +160,9 @@ struct virtnet_info {
>  	/* Ethtool settings */
>  	u8 duplex;
>  	u32 speed;
> +
> +	/* Headroom allocated in RX Queue */
> +	unsigned int headroom;
>  };
>  
>  struct padded_vnet_hdr {
> @@ -359,6 +363,7 @@ static void virtnet_xdp_xmit(struct virtnet_info *vi,
>  	}
>  
>  	if (vi->mergeable_rx_bufs) {
> +		xdp->data -= sizeof(struct virtio_net_hdr_mrg_rxbuf);
>  		/* Zero header and leave csum up to XDP layers */
>  		hdr = xdp->data;
>  		memset(hdr, 0, vi->hdr_len);
> @@ -375,7 +380,9 @@ static void virtnet_xdp_xmit(struct virtnet_info *vi,
>  		num_sg = 2;
>  		sg_init_table(sq->sg, 2);
>  		sg_set_buf(sq->sg, hdr, vi->hdr_len);
> -		skb_to_sgvec(skb, sq->sg + 1, 0, skb->len);
> +		skb_to_sgvec(skb, sq->sg + 1,
> +			     xdp->data - xdp->data_hard_start,
> +			     xdp->data_end - xdp->data);
>  	}
>  	err = virtqueue_add_outbuf(sq->vq, sq->sg, num_sg,
>  				   data, GFP_ATOMIC);
> @@ -401,7 +408,6 @@ static struct sk_buff *receive_small(struct net_device *dev,
>  	struct bpf_prog *xdp_prog;
>  
>  	len -= vi->hdr_len;
> -	skb_trim(skb, len);
>  
>  	rcu_read_lock();
>  	xdp_prog = rcu_dereference(rq->xdp_prog);
> @@ -413,11 +419,15 @@ static struct sk_buff *receive_small(struct net_device *dev,
>  		if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
>  			goto err_xdp;
>  
> -		xdp.data = skb->data;
> +		xdp.data_hard_start = skb->data;
> +		xdp.data = skb->data + vi->headroom;
>  		xdp.data_end = xdp.data + len;
>  		act = bpf_prog_run_xdp(xdp_prog, &xdp);
>  		switch (act) {
>  		case XDP_PASS:
> +			/* Recalculate length in case bpf program changed it */
> +			len = xdp.data_end - xdp.data;
> +			__skb_pull(skb, xdp.data - xdp.data_hard_start);
>  			break;
>  		case XDP_TX:
>  			virtnet_xdp_xmit(vi, rq, &xdp, skb);
> @@ -432,6 +442,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>  	}
>  	rcu_read_unlock();
>  
> +	skb_trim(skb, len);
>  	return skb;
>  
>  err_xdp:
> @@ -569,7 +580,11 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>  		if (unlikely(hdr->hdr.gso_type))
>  			goto err_xdp;
>  
> +		/* Allow consuming headroom but reserve enough space to push
> +		 * the descriptor on if we get an XDP_TX return code.
> +		 */
>  		data = page_address(xdp_page) + offset;
> +		xdp.data_hard_start = data - vi->headroom + desc_room;
>  		xdp.data = data + desc_room;
>  		xdp.data_end = xdp.data + (len - vi->hdr_len);
>  		act = bpf_prog_run_xdp(xdp_prog, &xdp);
> @@ -748,20 +763,21 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>  static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
>  			     gfp_t gfp)
>  {
> +	int headroom = GOOD_PACKET_LEN + vi->headroom;
>  	struct sk_buff *skb;
>  	struct virtio_net_hdr_mrg_rxbuf *hdr;
>  	int err;
>  
> -	skb = __netdev_alloc_skb_ip_align(vi->dev, GOOD_PACKET_LEN, gfp);
> +	skb = __netdev_alloc_skb_ip_align(vi->dev, headroom, gfp);
>  	if (unlikely(!skb))
>  		return -ENOMEM;
>  
> -	skb_put(skb, GOOD_PACKET_LEN);
> +	skb_put(skb, headroom);
>  
>  	hdr = skb_vnet_hdr(skb);
>  	sg_init_table(rq->sg, 2);
>  	sg_set_buf(rq->sg, hdr, vi->hdr_len);
> -	skb_to_sgvec(skb, rq->sg + 1, 0, skb->len);
> +	skb_to_sgvec(skb, rq->sg + 1, vi->headroom, skb->len - vi->headroom);
>  
>  	err = virtqueue_add_inbuf(rq->vq, rq->sg, 2, skb, gfp);
>  	if (err < 0)
> @@ -829,24 +845,27 @@ static unsigned int get_mergeable_buf_len(struct ewma_pkt_len *avg_pkt_len)
>  	return ALIGN(len, MERGEABLE_BUFFER_ALIGN);
>  }
>  
> -static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
> +static int add_recvbuf_mergeable(struct virtnet_info *vi,
> +				 struct receive_queue *rq, gfp_t gfp)
>  {
>  	struct page_frag *alloc_frag = &rq->alloc_frag;
> +	unsigned int headroom = vi->headroom;
>  	char *buf;
>  	unsigned long ctx;
>  	int err;
>  	unsigned int len, hole;
>  
>  	len = get_mergeable_buf_len(&rq->mrg_avg_pkt_len);
> -	if (unlikely(!skb_page_frag_refill(len, alloc_frag, gfp)))
> +	if (unlikely(!skb_page_frag_refill(len + headroom, alloc_frag, gfp)))
>  		return -ENOMEM;
>  
>  	buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> +	buf += headroom; /* advance address leaving hole at front of pkt */
>  	ctx = mergeable_buf_to_ctx(buf, len);
>  	get_page(alloc_frag->page);
> -	alloc_frag->offset += len;
> +	alloc_frag->offset += len + headroom;
>  	hole = alloc_frag->size - alloc_frag->offset;
> -	if (hole < len) {
> +	if (hole < len + headroom) {
>  		/* To avoid internal fragmentation, if there is very likely not
>  		 * enough space for another buffer, add the remaining space to
>  		 * the current buffer. This extra space is not included in
> @@ -880,7 +899,7 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
>  	gfp |= __GFP_COLD;
>  	do {
>  		if (vi->mergeable_rx_bufs)
> -			err = add_recvbuf_mergeable(rq, gfp);
> +			err = add_recvbuf_mergeable(vi, rq, gfp);
>  		else if (vi->big_packets)
>  			err = add_recvbuf_big(vi, rq, gfp);
>  		else
> @@ -1675,12 +1694,90 @@ static void virtnet_init_settings(struct net_device *dev)
>  	.set_settings = virtnet_set_settings,
>  };
>  
> +#define VIRTIO_XDP_HEADROOM 256
> +
> +static int init_vqs(struct virtnet_info *vi);
> +static void remove_vq_common(struct virtnet_info *vi, bool lock);
> +
> +/* Reset virtio device with RTNL held this is very similar to the
> + * freeze()/restore() logic except we need to ensure locking. It is
> + * possible that this routine may fail and leave the driver in a
> + * failed state. However assuming the driver negotiated correctly
> + * at probe time we _should_ be able to (re)negotiate driver again.
> + */
> +static int virtnet_xdp_reset(struct virtnet_info *vi)
> +{
> +	struct virtio_device *vdev = vi->vdev;
> +	unsigned int status;
> +	int i, ret;
> +
> +	/* Disable and unwind rings */
> +	virtio_config_disable(vdev);
> +	vdev->failed = vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_FAILED;
> +
> +	netif_device_detach(vi->dev);

After this point, netif_device_present
will return false, and then we have a bunch of code
that does
                if (!netif_device_present(dev))
                        return -ENODEV;


so we need to audit this code to make sure it's
all called under rtnl, correct?

We don't want it to fail because of timing.

Maybe add an assert there.


> +	cancel_delayed_work_sync(&vi->refill);
> +	if (netif_running(vi->dev)) {
> +		for (i = 0; i < vi->max_queue_pairs; i++)
> +			napi_disable(&vi->rq[i].napi);
> +	}
> +
> +	remove_vq_common(vi, false);
> +
> +	/* Do a reset per virtio spec recommendation */
> +	vdev->config->reset(vdev);
> +
> +	/* Acknowledge that we've seen the device. */
> +	status = vdev->config->get_status(vdev);
> +	vdev->config->set_status(vdev, status | VIRTIO_CONFIG_S_ACKNOWLEDGE);
> +
> +	/* Notify driver is up and finalize features per specification. The
> +	 * error code from finalize features is checked here but should not
> +	 * fail because we assume features were previously synced successfully.
> +	 */
> +	status = vdev->config->get_status(vdev);
> +	vdev->config->set_status(vdev, status | VIRTIO_CONFIG_S_DRIVER);
> +	ret = virtio_finalize_features(vdev);

I'd rather we put all this in virtio core, maybe call it virtio_reset or
something.

> +	if (ret) {
> +		netdev_warn(vi->dev, "virtio_finalize_features failed during reset aborting\n");
> +		goto err;
> +	}
> +
> +	ret = init_vqs(vi);
> +	if (ret) {
> +		netdev_warn(vi->dev, "init_vqs failed during reset aborting\n");
> +		goto err;
> +	}
> +	virtio_device_ready(vi->vdev);
> +
> +	if (netif_running(vi->dev)) {
> +		for (i = 0; i < vi->curr_queue_pairs; i++)
> +			if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> +				schedule_delayed_work(&vi->refill, 0);
> +
> +		for (i = 0; i < vi->max_queue_pairs; i++)
> +			virtnet_napi_enable(&vi->rq[i]);
> +	}
> +	netif_device_attach(vi->dev);
> +	/* Finally, tell the device we're all set */
> +	status = vdev->config->get_status(vdev);
> +	vdev->config->set_status(vdev, status | VIRTIO_CONFIG_S_DRIVER_OK);
> +	virtio_config_enable(vdev);
> +
> +	return 0;
> +err:
> +	status = vdev->config->get_status(vdev);
> +	vdev->config->set_status(vdev, status | VIRTIO_CONFIG_S_FAILED);

And maybe virtio_fail ?

> +	return ret;
> +}
> +
>  static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>  {
>  	unsigned long int max_sz = PAGE_SIZE - sizeof(struct padded_vnet_hdr);
>  	struct virtnet_info *vi = netdev_priv(dev);
>  	struct bpf_prog *old_prog;
>  	u16 xdp_qp = 0, curr_qp;
> +	unsigned int old_hr;
>  	int i, err;
>  
>  	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> @@ -1712,18 +1809,31 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>  		return -ENOMEM;
>  	}
>  
> -	err = virtnet_set_queues(vi, curr_qp + xdp_qp);
> -	if (err) {
> -		dev_warn(&dev->dev, "XDP Device queue allocation failure.\n");
> -		return err;
> -	}
> -
> +	old_hr = vi->headroom;
>  	if (prog) {
>  		prog = bpf_prog_add(prog, vi->max_queue_pairs - 1);
> -		if (IS_ERR(prog)) {
> -			virtnet_set_queues(vi, curr_qp);
> +		if (IS_ERR(prog))
>  			return PTR_ERR(prog);
> -		}
> +		vi->headroom = VIRTIO_XDP_HEADROOM;
> +	} else {
> +		vi->headroom = 0;
> +	}
> +
> +	/* Changing the headroom in buffers is a disruptive operation because
> +	 * existing buffers must be flushed and reallocated. This will happen
> +	 * when a xdp program is initially added or xdp is disabled by removing
> +	 * the xdp program.
> +	 */
> +	if (old_hr != vi->headroom) {
> +		err = virtnet_xdp_reset(vi);
> +		if (err)
> +			goto err_reset;
> +	}
> +
> +	err = virtnet_set_queues(vi, curr_qp + xdp_qp);
> +	if (err) {
> +		dev_warn(&dev->dev, "XDP Device queue allocation failure.\n");
> +		goto err_reset;
>  	}
>  
>  	vi->xdp_queue_pairs = xdp_qp;
> @@ -1737,6 +1847,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
>  	}
>  
>  	return 0;
> +err_reset:
> +	if (prog)
> +		bpf_prog_sub(prog, vi->max_queue_pairs - 1);
> +	vi->headroom = old_hr;
> +	return err;
>  }
>  
>  static bool virtnet_xdp_query(struct net_device *dev)
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 7062bb0..0e922b9 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -145,14 +145,15 @@ void virtio_config_changed(struct virtio_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(virtio_config_changed);
>  
> -static void virtio_config_disable(struct virtio_device *dev)
> +void virtio_config_disable(struct virtio_device *dev)
>  {
>  	spin_lock_irq(&dev->config_lock);
>  	dev->config_enabled = false;
>  	spin_unlock_irq(&dev->config_lock);
>  }
> +EXPORT_SYMBOL_GPL(virtio_config_disable);
>  
> -static void virtio_config_enable(struct virtio_device *dev)
> +void virtio_config_enable(struct virtio_device *dev)
>  {
>  	spin_lock_irq(&dev->config_lock);
>  	dev->config_enabled = true;
> @@ -161,8 +162,9 @@ static void virtio_config_enable(struct virtio_device *dev)
>  	dev->config_change_pending = false;
>  	spin_unlock_irq(&dev->config_lock);
>  }
> +EXPORT_SYMBOL_GPL(virtio_config_enable);
>  
> -static int virtio_finalize_features(struct virtio_device *dev)
> +int virtio_finalize_features(struct virtio_device *dev)
>  {
>  	int ret = dev->config->finalize_features(dev);
>  	unsigned status;
> @@ -182,6 +184,7 @@ static int virtio_finalize_features(struct virtio_device *dev)
>  	}
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(virtio_finalize_features);
>  
>  static int virtio_dev_probe(struct device *_d)
>  {
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index d5eb547..eac8f05 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -137,6 +137,9 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev)
>  
>  void virtio_break_device(struct virtio_device *dev);
>  
> +void virtio_config_disable(struct virtio_device *dev);
> +void virtio_config_enable(struct virtio_device *dev);
> +int virtio_finalize_features(struct virtio_device *dev);
>  void virtio_config_changed(struct virtio_device *dev);
>  #ifdef CONFIG_PM_SLEEP
>  int virtio_device_freeze(struct virtio_device *dev);

^ permalink raw reply

* Re: [RFC PATCH] tcp: accept RST for rcv_nxt - 1 after receiving a FIN
From: Jason Baron @ 2017-01-13 17:28 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1484149690.21472.53.camel@edumazet-glaptop3.roam.corp.google.com>

On 01/11/2017 10:48 AM, Eric Dumazet wrote:

> On Thu, 2017-01-05 at 16:33 -0500, Jason Baron wrote:
>
>>  
>> +/* Accept RST for rcv_nxt - 1 after a FIN.
>> + * When tcp connections are abruptly terminated from Mac OSX (via ^C), a
>> + * FIN is sent followed by a RST packet. The RST is sent with the same
>> + * sequence number as the FIN, and thus according to RFC 5961 a challenge
>> + * ACK should be sent. However, Mac OSX does not reply to the challenge ACK
>> + * with a RST on the closed socket, hence accept this class of RSTs.
>> + */
>> +static bool tcp_reset_check(struct sock *sk, struct sk_buff *skb)
> const struct sock *sk, const struct sk_buff *skb
>
>> +{
>> +	struct tcp_sock *tp = tcp_sk(sk);
>> +
>> +	return unlikely((TCP_SKB_CB(skb)->seq == (tp->rcv_nxt - 1)) &&
>> +			(TCP_SKB_CB(skb)->end_seq == (tp->rcv_nxt - 1))	&&
> Why is the test on end_seq needed ?

Hi,

(Re-sending - seems like my reply was lost)

I wanted to define this condition as narrowly as I could. I'm ok
dropping it -
I'm not sure its going to make much difference in practice. So to that end,
dropping this extra check makes sense.

I posted this as RFC because RFC 5961, I don't think says anything about
accepting rcv_nxt - 1 in this case, so I was wondering what people
thought...

Thanks,

-Jason

>> +			(sk->sk_state == TCP_CLOSE_WAIT ||
>> +			 sk->sk_state == TCP_LAST_ACK ||
>> +			 sk->sk_state == TCP_CLOSING));
>> +}
> Testing many states can be done more efficiently :
>
>    (1 << sk->sk_state) & (TCPF_CLOSE_WAIT | TCPF_LAST_ACK |
>                           TCPF_CLOSING)
>
> Thanks
>

^ permalink raw reply

* Re: [net PATCH v3 2/5] net: virtio: wrap rtnl_lock in test for calling with lock already held
From: John Fastabend @ 2017-01-13 17:31 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: jasowang, mst, john.r.fastabend, netdev, alexei.starovoitov,
	daniel
In-Reply-To: <20170113083408.202d6e30@xeon-e3>

On 17-01-13 08:34 AM, Stephen Hemminger wrote:
> On Thu, 12 Jan 2017 18:51:00 -0800
> John Fastabend <john.fastabend@gmail.com> wrote:
> 
>>  
>> -static void free_receive_bufs(struct virtnet_info *vi)
>> +static void free_receive_bufs(struct virtnet_info *vi, bool need_lock)
>>  {
>>  	struct bpf_prog *old_prog;
>>  	int i;
>>  
>> -	rtnl_lock();
>> +	if (need_lock)
>> +		rtnl_lock();
>>  	for (i = 0; i < vi->max_queue_pairs; i++) {
>>  		while (vi->rq[i].pages)
>>  			__free_pages(get_a_page(&vi->rq[i], GFP_KERNEL), 0);
>> @@ -1879,7 +1880,8 @@ static void free_receive_bufs(struct virtnet_info *vi)
>>  		if (old_prog)
>>  			bpf_prog_put(old_prog);
>>  	}
>> -	rtnl_unlock();
>> +	if (need_lock)
>> +		rtnl_unlock();
>>  }
> 
> Conditional locking is bad idea; sparse complains about it and is later source
> of bugs. The more typical way of doing this in kernel is:

OK I'll use the normal form.

> 
> void _foo(some args)
> {
> 	ASSERT_RTNL();
> 
> 	...
> }
> 
> void foo(some args)
> {
> 	rtnl_lock();
> 	_foo(some args)
> 	rtnl_unlock();
> }
> 
> 

^ permalink raw reply

* Re: [PATCH v2] tcp: fix tcp_fastopen unaligned access complaints on sparc
From: David Miller @ 2017-01-13 17:31 UTC (permalink / raw)
  To: shannon.nelson
  Cc: netdev, sparclinux, linux-kernel, eric.dumazet, rob.gardner
In-Reply-To: <1484259898-30719-1-git-send-email-shannon.nelson@oracle.com>

From: Shannon Nelson <shannon.nelson@oracle.com>
Date: Thu, 12 Jan 2017 14:24:58 -0800

> Fix up a data alignment issue on sparc by swapping the order
> of the cookie byte array field with the length field in
> struct tcp_fastopen_cookie, and making it a proper union
> to clean up the typecasting.
> 
> This addresses log complaints like these:
>     log_unaligned: 113 callbacks suppressed
>     Kernel unaligned access at TPC[976490] tcp_try_fastopen+0x2d0/0x360
>     Kernel unaligned access at TPC[9764ac] tcp_try_fastopen+0x2ec/0x360
>     Kernel unaligned access at TPC[9764c8] tcp_try_fastopen+0x308/0x360
>     Kernel unaligned access at TPC[9764e4] tcp_try_fastopen+0x324/0x360
>     Kernel unaligned access at TPC[976490] tcp_try_fastopen+0x2d0/0x360
> 
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> ---
> v2: Use Eric's suggestion for a union in the struct

Applied and queued up for -stable, thanks.

^ permalink raw reply

* Re: resend: tcp: performance issue with fastopen connections (mss > window)
From: Neal Cardwell @ 2017-01-13 17:32 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Alexey Kodanev, Eric Dumazet, David Miller, netdev,
	Vasily Isaenko, Yuchung Cheng
In-Reply-To: <CANn89i+5m2uUBmHRHG32_oPyR53vQoG4SfOY6xWG_RNxPz6dZw@mail.gmail.com>

On Fri, Jan 13, 2017 at 12:14 PM, Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Jan 13, 2017 at 9:07 AM, Alexey Kodanev
> <alexey.kodanev@oracle.com> wrote:
> > Hi Eric,
> > On 13.01.2017 18:35, Eric Dumazet wrote:
>
> >> Care to send a packetdrill test so that we have a clear picture of what
> >> is going on ?
> >
> > Is it capable of making two connections in the single test, one after
> > another?
>
> Absolutely.
>
> Neal, Yuchung would you be kind enough to send a Fastopen tpacketdrill
> template showing a typical fastopen test
> running on an upstream kernel ?
>
> Thanks !

Sure, here is an example packetdrill script, IIRC written by Yuchung,
which demonstrates TCP fast open and consecutive active connections:

`sysctl -q net.ipv4.tcp_timestamps=0`

// Cache warmup: send a Fast Open cookie request
0.000 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
   +0 fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK) = 0
   +0 sendto(3, ..., 0, MSG_FASTOPEN, ..., ...) = -1 EINPROGRESS
(Operation is now in progress)
   +0 > S 0:0(0) <mss 1460,nop,nop,sackOK,nop,wscale 8,FO,nop,nop>
+.010 < S. 123:123(0) ack 1 win 5840 <mss
1040,nop,nop,sackOK,nop,wscale 6,FO abcd1234,nop,nop>
   +0 > . 1:1(0) ack 1
   +0 close(3) = 0
   +0 > F. 1:1(0) ack 1
+.010 < F. 1:1(0) ack 2 win 92
   +0 > .  2:2(0) ack 2

//
// TEST1: Servers sends SYN-ACK with data and another two data packets
//
   +0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 4
   +0 fcntl(4, F_SETFL, O_RDWR|O_NONBLOCK) = 0
   +0 sendto(4, ..., 1000, MSG_FASTOPEN, ..., ...) = 1000
   +0 > S 0:1000(1000) <mss 1460,nop,nop,sackOK,nop,wscale 8,FO
abcd1234,nop,nop>
+.010 < S. 1000000:1001400(1400) ack 1001 win 5840 <mss
1040,nop,nop,sackOK,nop,wscale 6>
   +0 < . 1401:2801(1400) ack 1001 win 257
   +0 < P. 2801:3001(200) ack 1001 win 257

neal

^ permalink raw reply

* Re: Setting link down or up in software
From: Mason @ 2017-01-13 17:35 UTC (permalink / raw)
  To: Zefir Kurtisi, netdev
  Cc: Mans Rullgard, Florian Fainelli, Andrew Lunn, Thibaud Cornic
In-Reply-To: <bfe5bd8b-d4eb-8c8e-45d8-6ea1c133b4a7@neratec.com>

On 13/01/2017 17:28, Zefir Kurtisi wrote:

> As for your specific problem: since I fought myself with the PHY/ETH subsystems
> over the past months, I might remember something relevant to your issue. Could you
> give some more info on your setup (PHY driver, opmode (SGMII, RGMII, etc.), ETH).

Hello Zefir,

My boards are using these drivers:

http://lxr.free-electrons.com/source/drivers/net/ethernet/aurora/nb8800.c
http://lxr.free-electrons.com/source/drivers/net/phy/at803x.c

The relevant device tree nodes are:

		eth0: ethernet@26000 {
			compatible = "sigma,smp8734-ethernet";
			reg = <0x26000 0x800>;
			interrupts = <38 IRQ_TYPE_LEVEL_HIGH>;
			clocks = <&clkgen SYS_CLK>;
		};

&eth0 {
	phy-connection-type = "rgmii";
	phy-handle = <&eth0_phy>;
	#address-cells = <1>;
	#size-cells = <0>;

	/* Atheros AR8035 */
	eth0_phy: ethernet-phy@4 {
		compatible = "ethernet-phy-id004d.d072",
			     "ethernet-phy-ieee802.3-c22";
		interrupts = <37 IRQ_TYPE_EDGE_RISING>;
		reg = <4>;
	};
};

If I comment the PHY "interrupts" property, then the PHY framework
falls back to polling.

Am I forgetting important information?

Regards.

^ permalink raw reply

* Re: [PATCH net v3] ipv6: sr: fix several BUGs when preemption is enabled
From: David Miller @ 2017-01-13 17:30 UTC (permalink / raw)
  To: david.lebrun; +Cc: netdev
In-Reply-To: <1484253001-29677-1-git-send-email-david.lebrun@uclouvain.be>

From: David Lebrun <david.lebrun@uclouvain.be>
Date: Thu, 12 Jan 2017 21:30:01 +0100

> When CONFIG_PREEMPT=y, CONFIG_IPV6=m and CONFIG_SEG6_HMAC=y,
> seg6_hmac_init() is called during the initialization of the ipv6 module.
> This causes a subsequent call to smp_processor_id() with preemption
> enabled, resulting in the following trace.
 ...
> Moreover, dst_cache_* functions also call smp_processor_id(), generating
> a similar trace.
> 
> This patch uses raw_cpu_ptr() in seg6_hmac_init() rather than this_cpu_ptr()
> and disable preemption when using dst_cache_* functions.
> 
> Signed-off-by: David Lebrun <david.lebrun@uclouvain.be>

Applied, thanks.

^ permalink raw reply

* Re: resend: tcp: performance issue with fastopen connections (mss > window)
From: Eric Dumazet @ 2017-01-13 17:41 UTC (permalink / raw)
  To: Neal Cardwell
  Cc: Eric Dumazet, Alexey Kodanev, David Miller, netdev,
	Vasily Isaenko, Yuchung Cheng
In-Reply-To: <CADVnQykVj0CdHVYkfuogy6nOxfL+ODXue-3siKh_oACCBjVrtg@mail.gmail.com>

On Fri, 2017-01-13 at 12:32 -0500, Neal Cardwell wrote:
> On Fri, Jan 13, 2017 at 12:14 PM, Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Fri, Jan 13, 2017 at 9:07 AM, Alexey Kodanev
> > <alexey.kodanev@oracle.com> wrote:
> > > Hi Eric,
> > > On 13.01.2017 18:35, Eric Dumazet wrote:
> >
> > >> Care to send a packetdrill test so that we have a clear picture of what
> > >> is going on ?
> > >
> > > Is it capable of making two connections in the single test, one after
> > > another?
> >
> > Absolutely.
> >
> > Neal, Yuchung would you be kind enough to send a Fastopen tpacketdrill
> > template showing a typical fastopen test
> > running on an upstream kernel ?
> >
> > Thanks !
> 
> Sure, here is an example packetdrill script, IIRC written by Yuchung,
> which demonstrates TCP fast open and consecutive active connections:
> 
> `sysctl -q net.ipv4.tcp_timestamps=0`
> 
> // Cache warmup: send a Fast Open cookie request
> 0.000 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
>    +0 fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK) = 0
>    +0 sendto(3, ..., 0, MSG_FASTOPEN, ..., ...) = -1 EINPROGRESS
> (Operation is now in progress)
>    +0 > S 0:0(0) <mss 1460,nop,nop,sackOK,nop,wscale 8,FO,nop,nop>
> +.010 < S. 123:123(0) ack 1 win 5840 <mss
> 1040,nop,nop,sackOK,nop,wscale 6,FO abcd1234,nop,nop>
>    +0 > . 1:1(0) ack 1
>    +0 close(3) = 0
>    +0 > F. 1:1(0) ack 1
> +.010 < F. 1:1(0) ack 2 win 92
>    +0 > .  2:2(0) ack 2
> 
> //
> // TEST1: Servers sends SYN-ACK with data and another two data packets
> //
>    +0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 4
>    +0 fcntl(4, F_SETFL, O_RDWR|O_NONBLOCK) = 0
>    +0 sendto(4, ..., 1000, MSG_FASTOPEN, ..., ...) = 1000
>    +0 > S 0:1000(1000) <mss 1460,nop,nop,sackOK,nop,wscale 8,FO
> abcd1234,nop,nop>
> +.010 < S. 1000000:1001400(1400) ack 1001 win 5840 <mss
> 1040,nop,nop,sackOK,nop,wscale 6>
>    +0 < . 1401:2801(1400) ack 1001 win 257
>    +0 < P. 2801:3001(200) ack 1001 win 257
> 
> neal

Thanks Neal

Also worth adding that packetdrill has the following option to tune the
MTU on the tun device :

--mtu=xxxxx

^ permalink raw reply

* Re: [PATCH 3/3] powerpc: bpf: implement in-register swap for 64-bit endian operations
From: 'Naveen N. Rao' @ 2017-01-13 17:52 UTC (permalink / raw)
  To: David Laight
  Cc: mpe@ellerman.id.au, linuxppc-dev@lists.ozlabs.org,
	netdev@vger.kernel.org, ast@fb.com, daniel@iogearbox.net,
	davem@davemloft.net
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DB02635FB@AcuExch.aculab.com>

On 2017/01/13 05:17PM, David Laight wrote:
> From: Naveen N. Rao
> > Sent: 13 January 2017 17:10
> > Generate instructions to perform the endian conversion using registers,
> > rather than generating two memory accesses.
> > 
> > The "way easier and faster" comment was obviously for the author, not
> > the processor.
> 
> That rather depends on whether the processor has a store to load forwarder
> that will satisfy the read from the store buffer.
> I don't know about ppc, but at least some x86 will do that.

Interesting - good to know that.

However, I don't think powerpc does that and in-register swap is likely 
faster regardless. Note also that gcc prefers this form at higher 
optimization levels.

Thanks,
Naveen

^ permalink raw reply

* [PATCH v2] xen-netfront: Fix Rx stall during network stress and OOM
From: Remanan Pillai @ 2017-01-13 17:55 UTC (permalink / raw)
  To: boris.ostrovsky, jgross, xen-devel, netdev, linux-kernel
  Cc: Vineeth Remanan Pillai, kamatam, aliguori
In-Reply-To: <0cb06b48-cb3c-47aa-2ae6-3a70197a5b64@amazon.com>

From: Vineeth Remanan Pillai <vineethp@amazon.com>

During an OOM scenario, request slots could not be created as skb
allocation fails. So the netback cannot pass in packets and netfront
wrongly assumes that there is no more work to be done and it disables
polling. This causes Rx to stall.

The issue is with the retry logic which schedules the timer if the
created slots are less than NET_RX_SLOTS_MIN. The count of new request
slots to be pushed are calculated as a difference between new req_prod
and rsp_cons which could be more than the actual slots, if there are
unconsumed responses.

The fix is to calculate the count of newly created slots as the
difference between new req_prod and old req_prod.

Signed-off-by: Vineeth Remanan Pillai <vineethp@amazon.com>
---
Changes in v2:
	- Removed the old implementation of enabling polling on
	  skb allocation error.
	- Corrected the refill timer logic to schedule when newly
	  created slots since last push is less than NET_RX_SLOTS_MIN.

 drivers/net/xen-netfront.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 40f26b6..2c7c29f 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -321,7 +321,7 @@ static void xennet_alloc_rx_buffers(struct netfront_queue *queue)
 	queue->rx.req_prod_pvt = req_prod;
 
 	/* Not enough requests? Try again later. */
-	if (req_prod - queue->rx.rsp_cons < NET_RX_SLOTS_MIN) {
+	if (req_prod - queue->rx.sring->req_prod < NET_RX_SLOTS_MIN) {
 		mod_timer(&queue->rx_refill_timer, jiffies + (HZ/10));
 		return;
 	}
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related

* Re: [PATCH v2 1/2] bpf: add a longest prefix match trie map implementation
From: Alexei Starovoitov @ 2017-01-13 18:01 UTC (permalink / raw)
  To: Daniel Mack; +Cc: ast, dh.herrmann, daniel, netdev, davem
In-Reply-To: <20170112172922.16636-2-daniel@zonque.org>

On Thu, Jan 12, 2017 at 06:29:21PM +0100, Daniel Mack wrote:
> This trie implements a longest prefix match algorithm that can be used
> to match IP addresses to a stored set of ranges.
> 
> Internally, data is stored in an unbalanced trie of nodes that has a
> maximum height of n, where n is the prefixlen the trie was created
> with.
> 
> Tries may be created with prefix lengths that are multiples of 8, in
> the range from 8 to 2048. The key used for lookup and update operations
> is a struct bpf_lpm_trie_key, and the value is a uint64_t.
> 
> The code carries more information about the internal implementation.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  include/uapi/linux/bpf.h |   7 +
>  kernel/bpf/Makefile      |   2 +-
>  kernel/bpf/lpm_trie.c    | 499 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 507 insertions(+), 1 deletion(-)
>  create mode 100644 kernel/bpf/lpm_trie.c
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 0eb0e87..d564277 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -63,6 +63,12 @@ struct bpf_insn {
>  	__s32	imm;		/* signed immediate constant */
>  };
>  
> +/* Key of an a BPF_MAP_TYPE_LPM_TRIE entry */
> +struct bpf_lpm_trie_key {
> +	__u32	prefixlen;	/* up to 32 for AF_INET, 128 for AF_INET6 */
> +	__u8	data[0];	/* Arbitrary size */
> +};
> +
>  /* BPF syscall commands, see bpf(2) man-page for details. */
>  enum bpf_cmd {
>  	BPF_MAP_CREATE,
> @@ -89,6 +95,7 @@ enum bpf_map_type {
>  	BPF_MAP_TYPE_CGROUP_ARRAY,
>  	BPF_MAP_TYPE_LRU_HASH,
>  	BPF_MAP_TYPE_LRU_PERCPU_HASH,
> +	BPF_MAP_TYPE_LPM_TRIE,
>  };
>  
>  enum bpf_prog_type {
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 1276474..e1ce4f4 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -1,7 +1,7 @@
>  obj-y := core.o
>  
>  obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o
> -obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o
> +obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o
>  ifeq ($(CONFIG_PERF_EVENTS),y)
>  obj-$(CONFIG_BPF_SYSCALL) += stackmap.o
>  endif
> diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
> new file mode 100644
> index 0000000..7f6d47e
> --- /dev/null
> +++ b/kernel/bpf/lpm_trie.c
> @@ -0,0 +1,499 @@
> +/*
> + * Longest prefix match list implementation
> + *
> + * Copyright (c) 2016,2017 Daniel Mack
> + * Copyright (c) 2016 David Herrmann
> + *
> + * This file is subject to the terms and conditions of version 2 of the GNU
> + * General Public License.  See the file COPYING in the main directory of the
> + * Linux distribution for more details.
> + */
> +
> +#include <linux/bpf.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/vmalloc.h>
> +#include <net/ipv6.h>
> +
> +struct lpm_trie_node;
> +
> +struct lpm_trie_node {
> +	struct rcu_head rcu;
> +	struct lpm_trie_node __rcu	*child[2];
> +	void				*value;
> +	u32				prefixlen;
> +	u8				data[0];
> +};
> +
> +struct lpm_trie {
> +	struct bpf_map			map;
> +	struct lpm_trie_node __rcu	*root;
> +	size_t				n_entries;
> +	size_t				max_prefixlen;
> +	size_t				data_size;
> +	raw_spinlock_t			lock;
> +};
> +
> +/*
> + * This trie implements a longest prefix match algorithm that can be used to
> + * match IP addresses to a stored set of ranges.
> + *
> + * Data stored in @data of struct bpf_lpm_key and struct lpm_trie_node is
> + * interpreted as big endian, so data[0] stores the most significant byte.
> + *
> + * Match ranges are internally stored in instances of struct lpm_trie_node
> + * which each contain their prefix length as well as two pointers that may
> + * lead to more nodes containing more specific matches. Each node also stores
> + * a value that is defined by and returned to userspace via the update_elem
> + * and lookup functions.
> + *
> + * For instance, let's start with a trie that was created with a prefix length
> + * of 32, so it can be used for IPv4 addresses, and one single element that
> + * matches 192.168.0.0/16. The data array would hence contain
> + * [0xc0, 0xa8, 0x00, 0x00] in big-endian notation. This documentation will
> + * stick to IP-address notation for readability though.
> + *
> + * As the trie is empty initially, the new node (1) will be places as root
> + * node, denoted as (R) in the example below. As there are no other node, both
> + * child pointers are %NULL.
> + *
> + *              +----------------+
> + *              |       (1)  (R) |
> + *              | 192.168.0.0/16 |
> + *              |    value: 1    |
> + *              |   [0]    [1]   |
> + *              +----------------+
> + *
> + * Next, let's add a new node (2) matching 192.168.0.0/24. As there is already
> + * a node with the same data and a smaller prefix (ie, a less specific one),
> + * node (2) will become a child of (1). In child index depends on the next bit
> + * that is outside of that (1) matches, and that bit is 0, so (2) will be

s/outside of that/outside of what/ ?
or you meant something else?

> + * child[0] of (1):
> + *
> + *              +----------------+
> + *              |       (1)  (R) |
> + *              | 192.168.0.0/16 |
> + *              |    value: 1    |
> + *              |   [0]    [1]   |
> + *              +----------------+
> + *                   |
> + *    +----------------+
> + *    |       (2)      |
> + *    | 192.168.0.0/24 |
> + *    |    value: 2    |
> + *    |   [0]    [1]   |
> + *    +----------------+
> + *
> + * The child[1] slot of (1) could be filled with another node which has bit #17
> + * (the next bit after the ones that (1) matches on) set to 1. For instance,
> + * 192.168.128.0/24:
> + *
> + *              +----------------+
> + *              |       (1)  (R) |
> + *              | 192.168.0.0/16 |
> + *              |    value: 1    |
> + *              |   [0]    [1]   |
> + *              +----------------+
> + *                   |      |
> + *    +----------------+  +------------------+
> + *    |       (2)      |  |        (3)       |
> + *    | 192.168.0.0/24 |  | 192.168.128.0/24 |
> + *    |    value: 2    |  |     value: 3     |
> + *    |   [0]    [1]   |  |    [0]    [1]    |
> + *    +----------------+  +------------------+
> + *
> + * Let's add another node (4) to the game for 192.168.1.0/24. In order to place
> + * it, node (1) is looked at first, and because (4) of the semantics laid out
> + * above (bit #17 is 0), it would normally be attached to (1) as child[0].
> + * However, that slot is already allocated, so a new node is needed in between.
> + * That node is does not have a value attached to it and it will never be

s/node is does/node does/

> + * returned to users as result of a lookup. It is only there to differenciate

s/differenciate/differentiate/

> + * the traversal further. It will get a prefix as wide as necessary to
> + * distinguish its two children:
> + *
> + *                      +----------------+
> + *                      |       (1)  (R) |
> + *                      | 192.168.0.0/16 |
> + *                      |    value: 1    |
> + *                      |   [0]    [1]   |
> + *                      +----------------+
> + *                           |      |
> + *            +----------------+  +------------------+
> + *            |       (4)  (I) |  |        (3)       |
> + *            | 192.168.0.0/23 |  | 192.168.128.0/24 |
> + *            |    value: ---  |  |     value: 3     |
> + *            |   [0]    [1]   |  |    [0]    [1]    |
> + *            +----------------+  +------------------+
> + *                 |      |
> + *  +----------------+  +----------------+
> + *  |       (2)      |  |       (5)      |
> + *  | 192.168.0.0/24 |  | 192.168.1.0/24 |
> + *  |    value: 2    |  |     value: 5   |
> + *  |   [0]    [1]   |  |   [0]    [1]   |
> + *  +----------------+  +----------------+
> + *
> + * 192.168.1.1/32 would be a child of (5) etc.
> + *
> + * An intermediate node will be turned into a 'real' node on demand. In the
> + * example above, (4) would be re-used if 192.168.0.0/23 is added to the trie.
> + *
> + * A fully populated trie would have a height of 32 nodes, as the trie was
> + * created with a prefix length of 32.
> + *
> + * The lookup starts at the root node. If the current node matches and if there
> + * is a child that can be used to become more specific, the trie is traversed
> + * downwards. The last node in the traversal that is a non-intermediate one is
> + * returned.
> + */
> +
> +static inline int extract_bit(const u8 *data, size_t index)
> +{
> +	return !!(data[index / 8] & (1 << (7 - (index % 8))));
> +}
> +
> +/**
> + * longest_prefix_match() - determine the longest prefix
> + * @trie:	The trie to get internal sizes from
> + * @node:	The node to operate on
> + * @key:	The key to compare to @node
> + *
> + * Determine the longest prefix of @node that matches the bits in @key.
> + */
> +static size_t longest_prefix_match(const struct lpm_trie *trie,
> +				   const struct lpm_trie_node *node,
> +				   const struct bpf_lpm_trie_key *key)
> +{
> +	size_t prefixlen = 0;
> +	int i;
> +
> +	for (i = 0; i < trie->data_size; i++) {
> +		size_t b;
> +
> +		b = 8 - fls(node->data[i] ^ key->data[i]);
> +		prefixlen += b;
> +
> +		if (prefixlen >= node->prefixlen || prefixlen >= key->prefixlen)
> +			return min(node->prefixlen, key->prefixlen);
> +
> +		if (b < 8)
> +			break;
> +	}
> +
> +	return prefixlen;
> +}
> +
> +/* Called from syscall or from eBPF program */
> +static void *trie_lookup_elem(struct bpf_map *map, void *_key)
> +{
> +	struct lpm_trie_node *node, *found = NULL;
> +	struct bpf_lpm_trie_key *key = _key;
> +	struct lpm_trie *trie =
> +		container_of(map, struct lpm_trie, map);
> +
> +	/* Start walking the trie from the root node ... */
> +
> +	for (node = rcu_dereference(trie->root); node;) {
> +		unsigned int next_bit;
> +		size_t matchlen;
> +
> +		/*
> +		 * Determine the longest prefix of @node that matches @key.
> +		 * If it's the maximum possible prefix for this trie, we have
> +		 * an exact match and can return it directly.
> +		 */

useful comments, but not in networking style. bpf is using networking style.
please reformat.

> +		matchlen = longest_prefix_match(trie, node, key);
> +		if (matchlen == trie->max_prefixlen)
> +			return node->value;
> +
> +		/*
> +		 * If the number of bits that match is smaller than the prefix
> +		 * length of @node, bail out and return the node we have seen
> +		 * last in the traversal (ie, the parent).
> +		 */
> +		if (matchlen < node->prefixlen)
> +			break;
> +
> +		/*
> +		 * Consider this node as return candidate unless it is an
> +		 * artificially added intermediate one, in which case ->value
> +		 * is %NULL
> +		 */
> +		if (node->value)
> +			found = node;
> +
> +		/*
> +		 * If the node match is fully satisfied, let's see if we can
> +		 * become more specific. Determine the next bit in the key and
> +		 * traverse down.
> +		 */
> +		next_bit = extract_bit(key->data, node->prefixlen);
> +		node = rcu_dereference(node->child[next_bit]);
> +	}
> +
> +	return found ? found->value : NULL;
> +}
> +
> +static struct lpm_trie_node *lpm_trie_node_alloc(const struct lpm_trie *trie,
> +						 const void *value)
> +{
> +	struct lpm_trie_node *node;
> +	gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN;
> +
> +	node = kmalloc(sizeof(struct lpm_trie_node) + trie->data_size, gfp);
> +	if (!node)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (value) {
> +		node->value = kmemdup(value, trie->map.value_size, gfp);

can you make value to be part of the node? similar to how hash map is done?
that will help avoid 2nd allocation, will speedup insertion and will
help converting this code to user pre-allocated elements.
I suspect the concern was that for many inner nodes that value is null ?
But in your use case the value_size will be == 0 eventually,
so by embedding it when can save memory too, since 'value' pointer will
be replaced with boolean present flag ?
So potentially less memory and less cache misses?

Overall algorithm is indeed straightforward and simple which is great,
but I would still like to see some performance numbers. Looks like
the best case for single 32-bit element it needs 4 xors and compares
which is fine. For mostly populate trie it's 4xors * 32 depth
which is pretty good too, but cache misses on pointer walks may
kill performance unless we're hitting the same path all the time.
I think it's all acceptable due to simplicity of the implementation
which we may improve later if it turns out to be a bottle neck for
some use cases. We just need a baseline to have realistic expectations.

Thanks!

^ permalink raw reply

* [PATCH] net: constify mdiobb_ops structures
From: Bhumika Goyal @ 2017-01-13 18:02 UTC (permalink / raw)
  To: julia.lawall, pantelis.antoniou, vbordug, sergei.shtylyov,
	f.fainelli, netdev, linux-kernel, linuxppc-dev, linux-renesas-soc
  Cc: Bhumika Goyal

Declare mdiobb_ops structures as const as they are only stored in the
ops field of mdiobb_ctrl structures. This field is of type const, so
mdiobb_ops structures having this property can be declared const too.
Done using Coccinelle:

@r disable optional_qualifier@
identifier x;
position p;
@@
static struct mdiobb_ops x@p={...};

@ok@
struct bb_info bitbang;
struct ravb_private priv;
struct ax_device ax;
struct mdio_gpio_info bb;
identifier r.x;
position p;
@@
(
bitbang.ctrl.ops=&x@p
|
priv.mdiobb.ops=&x@p
|
ax.bb_ctrl.ops=&x@p
|
bb.ctrl.ops=&x@p
)

@bad@
position p != {r.p,ok.p};
identifier r.x;
@@
x@p

@depends on !bad disable optional_qualifier@
identifier r.x;
@@
+const
struct mdiobb_ops x;

Before and after size details:

File size before:
   text	   data	    bss	    dec	    hex	filename
  13293	   1203	     16	  14512	   38b0 net/ethernet/8390/ax88796.o
File size after:
   text	   data	    bss	    dec	    hex	filename
  13357	   1139	     16	  14512	   38b0 net/ethernet/8390/ax88796.o

File size before: 
   text	   data	    bss	    dec	    hex	filename
   1440	    100	      0	   1540	    604 freescale/fs_enet/mii-bitbang.o
File size after: Remains the same

File size before:
   text	   data	    bss	    dec	    hex	filename
  19000	    192	     16	  19208	   4b08	net/ethernet/renesas/ravb_main.o
File size after: Remains the same

File size before:
   text	   data	    bss	    dec	    hex	filename
  28501	   1568	      8	  30077	   757d net/ethernet/renesas/sh_eth.o
File size after:
   text	   data	    bss	    dec	    hex	filename
  28565	   1504	      8	  30077	   757d net/ethernet/renesas/sh_eth.o

File size before:
   text	   data	    bss	    dec	    hex	filename
   1859	    248	      0	   2107	    83b	drivers/net/phy/mdio-gpio.o
File size after:
   text	   data	    bss	    dec	    hex	filename
   1915	    192	      0	   2107	    83b	drivers/net/phy/mdio-gpio.o

Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
---
 drivers/net/ethernet/8390/ax88796.c                  | 2 +-
 drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c | 2 +-
 drivers/net/ethernet/renesas/ravb_main.c             | 2 +-
 drivers/net/ethernet/renesas/sh_eth.c                | 2 +-
 drivers/net/phy/mdio-gpio.c                          | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/8390/ax88796.c b/drivers/net/ethernet/8390/ax88796.c
index b0a3b85..e977671 100644
--- a/drivers/net/ethernet/8390/ax88796.c
+++ b/drivers/net/ethernet/8390/ax88796.c
@@ -585,7 +585,7 @@ static int ax_bb_get_data(struct mdiobb_ctrl *ctrl)
 	return reg_memr & AX_MEMR_MDI ? 1 : 0;
 }
 
-static struct mdiobb_ops bb_ops = {
+static const struct mdiobb_ops bb_ops = {
 	.owner = THIS_MODULE,
 	.set_mdc = ax_bb_mdc,
 	.set_mdio_dir = ax_bb_dir,
diff --git a/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c b/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
index 1f015ed..c8e5d88 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
@@ -100,7 +100,7 @@ static inline void mdc(struct mdiobb_ctrl *ctrl, int what)
 	in_be32(bitbang->dat);
 }
 
-static struct mdiobb_ops bb_ops = {
+static const struct mdiobb_ops bb_ops = {
 	.owner = THIS_MODULE,
 	.set_mdc = mdc,
 	.set_mdio_dir = mdio_dir,
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 92d7692..1b0acd1 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -171,7 +171,7 @@ static int ravb_get_mdio_data(struct mdiobb_ctrl *ctrl)
 }
 
 /* MDIO bus control struct */
-static struct mdiobb_ops bb_ops = {
+static const struct mdiobb_ops bb_ops = {
 	.owner = THIS_MODULE,
 	.set_mdc = ravb_set_mdc,
 	.set_mdio_dir = ravb_set_mdio_dir,
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 00fafab..6ef5dd8 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1052,7 +1052,7 @@ static void sh_mdc_ctrl(struct mdiobb_ctrl *ctrl, int bit)
 }
 
 /* mdio bus control struct */
-static struct mdiobb_ops bb_ops = {
+static const struct mdiobb_ops bb_ops = {
 	.owner = THIS_MODULE,
 	.set_mdc = sh_mdc_ctrl,
 	.set_mdio_dir = sh_mmd_ctrl,
diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 27ab630..97333d5 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -122,7 +122,7 @@ static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
 	gpio_set_value_cansleep(bitbang->mdc, what ^ bitbang->mdc_active_low);
 }
 
-static struct mdiobb_ops mdio_gpio_ops = {
+static const struct mdiobb_ops mdio_gpio_ops = {
 	.owner = THIS_MODULE,
 	.set_mdc = mdc_set,
 	.set_mdio_dir = mdio_dir,
-- 
1.9.1

^ permalink raw reply related

* [PATCH net-next 0/2] mpls: Packet stats
From: Robert Shearman @ 2017-01-13 18:14 UTC (permalink / raw)
  To: davem; +Cc: netdev, roopa, David Ahern, ebiederm, Robert Shearman

This patchset records per-interface packet stats in the MPLS
forwarding path and exports them using a nest of attributes root at a
new IFLA_STATS_AF_SPEC attribute as part of RTM_GETSTATS messages:

[IFLA_STATS_AF_SPEC]
 -> [AF_MPLS]
  -> [MPLS_STATS_LINK]
   -> struct mpls_link_stats

The first patch adds the rtnl infrastructure for this, including a new
callbacks to per-AF ops of fill_stats_af and get_stats_af_size. The
second patch records MPLS stats and makes use of the infrastructure to
export them. The rtnl infrastructure could also be used to export IPv6
stats in the future.

Robert Shearman (2):
  net: AF-specific RTM_GETSTATS attributes
  mpls: Packet stats

 include/net/rtnetlink.h      |   4 +
 include/uapi/linux/if_link.h |   1 +
 include/uapi/linux/mpls.h    |  30 ++++++++
 net/core/rtnetlink.c         |  50 ++++++++++++
 net/mpls/af_mpls.c           | 179 +++++++++++++++++++++++++++++++++++++------
 net/mpls/internal.h          |  58 +++++++++++++-
 net/mpls/mpls_iptunnel.c     |  11 ++-
 7 files changed, 305 insertions(+), 28 deletions(-)

-- 
2.1.4

^ permalink raw reply

* [PATCH net-next 1/2] net: AF-specific RTM_GETSTATS attributes
From: Robert Shearman @ 2017-01-13 18:14 UTC (permalink / raw)
  To: davem; +Cc: netdev, roopa, David Ahern, ebiederm, Robert Shearman
In-Reply-To: <1484331253-5908-1-git-send-email-rshearma@brocade.com>

Add the functionality for including address-family-specific per-link
stats in RTM_GETSTATS messages. This is done through adding a new
IFLA_STATS_AF_SPEC attribute under which address family attributes are
nested and then the AF-specific attributes can be further nested. This
follows the model of IFLA_AF_SPEC on RTM_*LINK messages and it has the
advantage of presenting an easily extended hierarchy. The rtnl_af_ops
structure is extended to provide AFs with the opportunity to fill and
provide the size of their stats attributes.

One alternative would have been to provide AFs with the ability to add
attributes directly into the RTM_GETSTATS message without a nested
hierarchy. I discounted this approach as it increases the rate at
which the 32 attribute number space is used up and it makes
implementation a little more tricky for stats dump resuming (at the
moment the order in which attributes are added to the message has to
match the numeric order of the attributes).

Another alternative would have been to register per-AF RTM_GETSTATS
handlers. I discounted this approach as I perceived a common use-case
to be getting all the stats for an interface and this approach would
necessitate multiple requests/dumps to retrieve them all.

Signed-off-by: Robert Shearman <rshearma@brocade.com>
---
 include/net/rtnetlink.h      |  4 ++++
 include/uapi/linux/if_link.h |  1 +
 net/core/rtnetlink.c         | 50 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 4113916cc1bb..106de5f7bf06 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -139,6 +139,10 @@ struct rtnl_af_ops {
 						    const struct nlattr *attr);
 	int			(*set_link_af)(struct net_device *dev,
 					       const struct nlattr *attr);
+
+	int			(*fill_stats_af)(struct sk_buff *skb,
+						 const struct net_device *dev);
+	size_t			(*get_stats_af_size)(const struct net_device *dev);
 };
 
 void __rtnl_af_unregister(struct rtnl_af_ops *ops);
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 6b13e591abc9..184b16ed2b84 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -847,6 +847,7 @@ enum {
 	IFLA_STATS_LINK_XSTATS,
 	IFLA_STATS_LINK_XSTATS_SLAVE,
 	IFLA_STATS_LINK_OFFLOAD_XSTATS,
+	IFLA_STATS_AF_SPEC,
 	__IFLA_STATS_MAX,
 };
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 18b5aae99bec..4edc1bd7a735 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3829,6 +3829,39 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
 		*idxattr = 0;
 	}
 
+	if (stats_attr_valid(filter_mask, IFLA_STATS_AF_SPEC, *idxattr)) {
+		struct rtnl_af_ops *af_ops;
+
+		*idxattr = IFLA_STATS_AF_SPEC;
+		attr = nla_nest_start(skb, IFLA_STATS_AF_SPEC);
+		if (!attr)
+			goto nla_put_failure;
+
+		list_for_each_entry(af_ops, &rtnl_af_ops, list) {
+			if (af_ops->fill_stats_af) {
+				struct nlattr *af;
+				int err;
+
+				af = nla_nest_start(skb, af_ops->family);
+				if (!af)
+					goto nla_put_failure;
+
+				err = af_ops->fill_stats_af(skb, dev);
+
+				if (err == -ENODATA)
+					nla_nest_cancel(skb, af);
+				else if (err < 0)
+					goto nla_put_failure;
+
+				nla_nest_end(skb, af);
+			}
+		}
+
+		nla_nest_end(skb, attr);
+
+		*idxattr = 0;
+	}
+
 	nlmsg_end(skb, nlh);
 
 	return 0;
@@ -3885,6 +3918,23 @@ static size_t if_nlmsg_stats_size(const struct net_device *dev,
 	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS, 0))
 		size += rtnl_get_offload_stats_size(dev);
 
+	if (stats_attr_valid(filter_mask, IFLA_STATS_AF_SPEC, 0)) {
+		struct rtnl_af_ops *af_ops;
+
+		/* for IFLA_STATS_AF_SPEC */
+		size += nla_total_size(0);
+
+		list_for_each_entry(af_ops, &rtnl_af_ops, list) {
+			if (af_ops->get_stats_af_size) {
+				size += nla_total_size(
+					af_ops->get_stats_af_size(dev));
+
+				/* for AF_* */
+				size += nla_total_size(0);
+			}
+		}
+	}
+
 	return size;
 }
 
-- 
2.1.4

^ permalink raw reply related

* [PATCH net-next 2/2] mpls: Packet stats
From: Robert Shearman @ 2017-01-13 18:14 UTC (permalink / raw)
  To: davem; +Cc: netdev, roopa, David Ahern, ebiederm, Robert Shearman
In-Reply-To: <1484331253-5908-1-git-send-email-rshearma@brocade.com>

Having MPLS packet stats is useful for observing network operation and
for diagnosing network problems. In the absence of anything better,
RFC2863 and RFC3813 are used for guidance for which stats to expose
and the semantics of them. In particular rx_noroutes maps to in
unknown protos in RFC2863. The stats are exposed to userspace via
AF_MPLS attributes embedded in the IFLA_STATS_AF_SPEC attribute of
RTM_GETSTATS messages.

All the introduced fields are 64-bit, even error ones, to ensure no
overflow with long uptimes. Per-CPU counters are used to avoid
cache-line contention on the commonly used fields. The other fields
have also been made per-CPU for code to avoid performance problems in
error conditions on the assumption that on some platforms the cost of
atomic operations could be more expensive than sending the packet
(which is what would be done in the success case). If that's not the
case, we could instead not use per-CPU counters for these fields.

Only unicast and non-fragment are exposed at the moment, but other
counters can be exposed in the future either by adding to the end of
struct mpls_link_stats or by additional netlink attributes in the
AF_MPLS IFLA_STATS_AF_SPEC nested attribute.

Signed-off-by: Robert Shearman <rshearma@brocade.com>
---
 include/uapi/linux/mpls.h |  30 ++++++++
 net/mpls/af_mpls.c        | 179 ++++++++++++++++++++++++++++++++++++++++------
 net/mpls/internal.h       |  58 ++++++++++++++-
 net/mpls/mpls_iptunnel.c  |  11 ++-
 4 files changed, 250 insertions(+), 28 deletions(-)

diff --git a/include/uapi/linux/mpls.h b/include/uapi/linux/mpls.h
index 24a6cb1aec86..77a19dfe3990 100644
--- a/include/uapi/linux/mpls.h
+++ b/include/uapi/linux/mpls.h
@@ -43,4 +43,34 @@ struct mpls_label {
 
 #define MPLS_LABEL_FIRST_UNRESERVED	16 /* RFC3032 */
 
+/* These are embedded into IFLA_STATS_AF_SPEC:
+ * [IFLA_STATS_AF_SPEC]
+ * -> [AF_MPLS]
+ *    -> [MPLS_STATS_xxx]
+ *
+ * Attributes:
+ * [MPLS_STATS_LINK] = {
+ *     struct mpls_link_stats
+ * }
+ */
+enum {
+	MPLS_STATS_UNSPEC, /* also used as 64bit pad attribute */
+	MPLS_STATS_LINK,
+	__MPLS_STATS_MAX,
+};
+
+#define MPLS_STATS_MAX (__MPLS_STATS_MAX - 1)
+
+struct mpls_link_stats {
+	__u64	rx_packets;		/* total packets received	*/
+	__u64	tx_packets;		/* total packets transmitted	*/
+	__u64	rx_bytes;		/* total bytes received		*/
+	__u64	tx_bytes;		/* total bytes transmitted	*/
+	__u64	rx_errors;		/* bad packets received		*/
+	__u64	tx_errors;		/* packet transmit problems	*/
+	__u64	rx_dropped;		/* packet dropped on receive	*/
+	__u64	tx_dropped;		/* packet dropped on transmit	*/
+	__u64	rx_noroute;		/* no route for packet dest	*/
+};
+
 #endif /* _UAPI_MPLS_H */
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 15fe97644ffe..fb20941cdda2 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -8,6 +8,7 @@
 #include <linux/ipv6.h>
 #include <linux/mpls.h>
 #include <linux/vmalloc.h>
+#include <linux/percpu.h>
 #include <net/ip.h>
 #include <net/dst.h>
 #include <net/sock.h>
@@ -17,8 +18,8 @@
 #include <net/netns/generic.h>
 #if IS_ENABLED(CONFIG_IPV6)
 #include <net/ipv6.h>
-#include <net/addrconf.h>
 #endif
+#include <net/addrconf.h>
 #include <net/nexthop.h>
 #include "internal.h"
 
@@ -48,11 +49,6 @@ static struct mpls_route *mpls_route_input_rcu(struct net *net, unsigned index)
 	return rt;
 }
 
-static inline struct mpls_dev *mpls_dev_get(const struct net_device *dev)
-{
-	return rcu_dereference_rtnl(dev->mpls_ptr);
-}
-
 bool mpls_output_possible(const struct net_device *dev)
 {
 	return dev && (dev->flags & IFF_UP) && netif_carrier_ok(dev);
@@ -98,6 +94,29 @@ bool mpls_pkt_too_big(const struct sk_buff *skb, unsigned int mtu)
 }
 EXPORT_SYMBOL_GPL(mpls_pkt_too_big);
 
+void mpls_stats_inc_outucastpkts(struct net_device *dev,
+				 const struct sk_buff *skb)
+{
+	struct mpls_dev *mdev;
+	struct inet6_dev *in6dev;
+
+	if (skb->protocol == htons(ETH_P_MPLS_UC)) {
+		mdev = mpls_dev_get(dev);
+		if (mdev)
+			MPLS_INC_STATS_LEN(mdev, skb->len,
+					   tx_packets,
+					   tx_bytes);
+	} else if (skb->protocol == htons(ETH_P_IP)) {
+		IP_UPD_PO_STATS(dev_net(dev), IPSTATS_MIB_OUT, skb->len);
+	} else if (skb->protocol == htons(ETH_P_IPV6)) {
+		in6dev = __in6_dev_get(dev);
+		if (in6dev)
+			IP6_UPD_PO_STATS(dev_net(dev), in6dev,
+					 IPSTATS_MIB_OUT, skb->len);
+	}
+}
+EXPORT_SYMBOL_GPL(mpls_stats_inc_outucastpkts);
+
 static u32 mpls_multipath_hash(struct mpls_route *rt,
 			       struct sk_buff *skb, bool bos)
 {
@@ -253,6 +272,7 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev,
 	struct mpls_nh *nh;
 	struct mpls_entry_decoded dec;
 	struct net_device *out_dev;
+	struct mpls_dev *out_mdev;
 	struct mpls_dev *mdev;
 	unsigned int hh_len;
 	unsigned int new_header_size;
@@ -262,17 +282,25 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev,
 	/* Careful this entire function runs inside of an rcu critical section */
 
 	mdev = mpls_dev_get(dev);
-	if (!mdev || !mdev->input_enabled)
+	if (!mdev)
 		goto drop;
 
-	if (skb->pkt_type != PACKET_HOST)
+	MPLS_INC_STATS_LEN(mdev, skb->len, rx_packets,
+			   rx_bytes);
+
+	if (!mdev->input_enabled) {
+		MPLS_INC_STATS(mdev, rx_dropped);
 		goto drop;
+	}
+
+	if (skb->pkt_type != PACKET_HOST)
+		goto err;
 
 	if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL)
-		goto drop;
+		goto err;
 
 	if (!pskb_may_pull(skb, sizeof(*hdr)))
-		goto drop;
+		goto err;
 
 	/* Read and decode the label */
 	hdr = mpls_hdr(skb);
@@ -285,33 +313,35 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev,
 	skb_orphan(skb);
 
 	rt = mpls_route_input_rcu(net, dec.label);
-	if (!rt)
+	if (!rt) {
+		MPLS_INC_STATS(mdev, rx_noroute);
 		goto drop;
+	}
 
 	nh = mpls_select_multipath(rt, skb, dec.bos);
 	if (!nh)
-		goto drop;
-
-	/* Find the output device */
-	out_dev = rcu_dereference(nh->nh_dev);
-	if (!mpls_output_possible(out_dev))
-		goto drop;
+		goto err;
 
 	if (skb_warn_if_lro(skb))
-		goto drop;
+		goto err;
 
 	skb_forward_csum(skb);
 
 	/* Verify ttl is valid */
 	if (dec.ttl <= 1)
-		goto drop;
+		goto err;
 	dec.ttl -= 1;
 
+	/* Find the output device */
+	out_dev = rcu_dereference(nh->nh_dev);
+	if (!mpls_output_possible(out_dev))
+		goto tx_err;
+
 	/* Verify the destination can hold the packet */
 	new_header_size = mpls_nh_header_size(nh);
 	mtu = mpls_dev_mtu(out_dev);
 	if (mpls_pkt_too_big(skb, mtu - new_header_size))
-		goto drop;
+		goto tx_err;
 
 	hh_len = LL_RESERVED_SPACE(out_dev);
 	if (!out_dev->header_ops)
@@ -319,7 +349,7 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev,
 
 	/* Ensure there is enough space for the headers in the skb */
 	if (skb_cow(skb, hh_len + new_header_size))
-		goto drop;
+		goto tx_err;
 
 	skb->dev = out_dev;
 	skb->protocol = htons(ETH_P_MPLS_UC);
@@ -327,7 +357,7 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev,
 	if (unlikely(!new_header_size && dec.bos)) {
 		/* Penultimate hop popping */
 		if (!mpls_egress(rt, skb, dec))
-			goto drop;
+			goto err;
 	} else {
 		bool bos;
 		int i;
@@ -343,6 +373,8 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev,
 		}
 	}
 
+	mpls_stats_inc_outucastpkts(out_dev, skb);
+
 	/* If via wasn't specified then send out using device address */
 	if (nh->nh_via_table == MPLS_NEIGH_TABLE_UNSPEC)
 		err = neigh_xmit(NEIGH_LINK_TABLE, out_dev,
@@ -355,6 +387,13 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev,
 				    __func__, err);
 	return 0;
 
+tx_err:
+	out_mdev = out_dev ? mpls_dev_get(out_dev) : NULL;
+	if (out_mdev)
+		MPLS_INC_STATS(out_mdev, tx_errors);
+	goto drop;
+err:
+	MPLS_INC_STATS(mdev, rx_errors);
 drop:
 	kfree_skb(skb);
 	return NET_RX_DROP;
@@ -853,6 +892,70 @@ static int mpls_route_del(struct mpls_route_config *cfg)
 	return err;
 }
 
+static void mpls_get_stats(struct mpls_dev *mdev,
+			   struct mpls_link_stats *stats)
+{
+	struct mpls_pcpu_stats *p;
+	int i;
+
+	memset(stats, 0, sizeof(*stats));
+
+	for_each_possible_cpu(i) {
+		struct mpls_link_stats local;
+		unsigned int start;
+
+		p = per_cpu_ptr(mdev->stats, i);
+		do {
+			start = u64_stats_fetch_begin(&p->syncp);
+			local = p->stats;
+		} while (u64_stats_fetch_retry(&p->syncp, start));
+
+		stats->rx_packets	+= local.rx_packets;
+		stats->rx_bytes		+= local.rx_bytes;
+		stats->tx_packets	+= local.tx_packets;
+		stats->tx_bytes		+= local.tx_bytes;
+		stats->rx_errors	+= local.rx_errors;
+		stats->tx_errors	+= local.tx_errors;
+		stats->rx_dropped	+= local.rx_dropped;
+		stats->tx_dropped	+= local.tx_dropped;
+		stats->rx_noroute	+= local.rx_noroute;
+	}
+}
+
+static int mpls_fill_stats_af(struct sk_buff *skb,
+			      const struct net_device *dev)
+{
+	struct mpls_link_stats *stats;
+	struct mpls_dev *mdev;
+	struct nlattr *nla;
+
+	mdev = mpls_dev_get(dev);
+	if (!mdev)
+		return -ENODATA;
+
+	nla = nla_reserve_64bit(skb, MPLS_STATS_LINK,
+				sizeof(struct mpls_link_stats),
+				MPLS_STATS_UNSPEC);
+	if (!nla)
+		return -EMSGSIZE;
+
+	stats = nla_data(nla);
+	mpls_get_stats(mdev, stats);
+
+	return 0;
+}
+
+static size_t mpls_get_stats_af_size(const struct net_device *dev)
+{
+	struct mpls_dev *mdev;
+
+	mdev = mpls_dev_get(dev);
+	if (!mdev)
+		return 0;
+
+	return nla_total_size_64bit(sizeof(struct mpls_link_stats));
+}
+
 #define MPLS_PERDEV_SYSCTL_OFFSET(field)	\
 	(&((struct mpls_dev *)0)->field)
 
@@ -911,6 +1014,7 @@ static struct mpls_dev *mpls_add_dev(struct net_device *dev)
 {
 	struct mpls_dev *mdev;
 	int err = -ENOMEM;
+	int i;
 
 	ASSERT_RTNL();
 
@@ -918,6 +1022,17 @@ static struct mpls_dev *mpls_add_dev(struct net_device *dev)
 	if (!mdev)
 		return ERR_PTR(err);
 
+	mdev->stats = alloc_percpu(struct mpls_pcpu_stats);
+	if (!mdev->stats)
+		goto free;
+
+	for_each_possible_cpu(i) {
+		struct mpls_pcpu_stats *mpls_stats;
+
+		mpls_stats = per_cpu_ptr(mdev->stats, i);
+		u64_stats_init(&mpls_stats->syncp);
+	}
+
 	err = mpls_dev_sysctl_register(dev, mdev);
 	if (err)
 		goto free;
@@ -927,10 +1042,19 @@ static struct mpls_dev *mpls_add_dev(struct net_device *dev)
 	return mdev;
 
 free:
+	free_percpu(mdev->stats);
 	kfree(mdev);
 	return ERR_PTR(err);
 }
 
+static void mpls_dev_destroy_rcu(struct rcu_head *head)
+{
+	struct mpls_dev *mdev = container_of(head, struct mpls_dev, rcu);
+
+	free_percpu(mdev->stats);
+	kfree(mdev);
+}
+
 static void mpls_ifdown(struct net_device *dev, int event)
 {
 	struct mpls_route __rcu **platform_label;
@@ -1045,7 +1169,7 @@ static int mpls_dev_notify(struct notifier_block *this, unsigned long event,
 		if (mdev) {
 			mpls_dev_sysctl_unregister(mdev);
 			RCU_INIT_POINTER(dev->mpls_ptr, NULL);
-			kfree_rcu(mdev, rcu);
+			call_rcu(&mdev->rcu, mpls_dev_destroy_rcu);
 		}
 		break;
 	case NETDEV_CHANGENAME:
@@ -1706,6 +1830,12 @@ static struct pernet_operations mpls_net_ops = {
 	.exit = mpls_net_exit,
 };
 
+static struct rtnl_af_ops mpls_af_ops __read_mostly = {
+	.family		   = AF_MPLS,
+	.fill_stats_af	   = mpls_fill_stats_af,
+	.get_stats_af_size = mpls_get_stats_af_size,
+};
+
 static int __init mpls_init(void)
 {
 	int err;
@@ -1722,6 +1852,8 @@ static int __init mpls_init(void)
 
 	dev_add_pack(&mpls_packet_type);
 
+	rtnl_af_register(&mpls_af_ops);
+
 	rtnl_register(PF_MPLS, RTM_NEWROUTE, mpls_rtm_newroute, NULL, NULL);
 	rtnl_register(PF_MPLS, RTM_DELROUTE, mpls_rtm_delroute, NULL, NULL);
 	rtnl_register(PF_MPLS, RTM_GETROUTE, NULL, mpls_dump_routes, NULL);
@@ -1738,6 +1870,7 @@ module_init(mpls_init);
 static void __exit mpls_exit(void)
 {
 	rtnl_unregister_all(PF_MPLS);
+	rtnl_af_unregister(&mpls_af_ops);
 	dev_remove_pack(&mpls_packet_type);
 	unregister_netdevice_notifier(&mpls_dev_notifier);
 	unregister_pernet_subsys(&mpls_net_ops);
diff --git a/net/mpls/internal.h b/net/mpls/internal.h
index bdfef6c3271a..d97243034605 100644
--- a/net/mpls/internal.h
+++ b/net/mpls/internal.h
@@ -9,13 +9,58 @@ struct mpls_entry_decoded {
 	u8 bos;
 };
 
+struct mpls_pcpu_stats {
+	struct mpls_link_stats	stats;
+	struct u64_stats_sync	syncp;
+};
+
 struct mpls_dev {
-	int			input_enabled;
+	int				input_enabled;
 
-	struct ctl_table_header *sysctl;
-	struct rcu_head		rcu;
+	struct mpls_pcpu_stats __percpu	*stats;
+
+	struct ctl_table_header		*sysctl;
+	struct rcu_head			rcu;
 };
 
+#if BITS_PER_LONG == 32
+
+#define MPLS_INC_STATS_LEN(mdev, len, pkts_field, bytes_field)		\
+	do {								\
+		__typeof__(*(mdev)->stats) *ptr =			\
+			raw_cpu_ptr((mdev)->stats);			\
+		local_bh_disable();					\
+		u64_stats_update_begin(&ptr->syncp);			\
+		ptr->stats.pkts_field++;				\
+		ptr->stats.bytes_field += (len);			\
+		u64_stats_update_end(&ptr->syncp);			\
+		local_bh_enable();					\
+	} while (0)
+
+#define MPLS_INC_STATS(mdev, field)					\
+	do {								\
+		__typeof__(*(mdev)->stats) *ptr =			\
+			raw_cpu_ptr((mdev)->stats);			\
+		local_bh_disable();					\
+		u64_stats_update_begin(&ptr->syncp);			\
+		ptr->stats.field++;					\
+		u64_stats_update_end(&ptr->syncp);			\
+		local_bh_enable();					\
+	} while (0)
+
+#else
+
+#define MPLS_INC_STATS_LEN(mdev, len, pkts_field, bytes_field)		\
+	do {								\
+		this_cpu_inc((mdev)->stats->stats.pkts_field);		\
+		this_cpu_add((mdev)->stats->stats.bytes_field, (len));	\
+	} while (0)
+
+#define MPLS_INC_STATS(mdev, field)			\
+	this_cpu_inc((mdev)->stats->stats.field)
+
+#endif
+
 struct sk_buff;
 
 #define LABEL_NOT_SPECIFIED (1 << 20)
@@ -114,6 +159,11 @@ static inline struct mpls_entry_decoded mpls_entry_decode(struct mpls_shim_hdr *
 	return result;
 }
 
+static inline struct mpls_dev *mpls_dev_get(const struct net_device *dev)
+{
+	return rcu_dereference_rtnl(dev->mpls_ptr);
+}
+
 int nla_put_labels(struct sk_buff *skb, int attrtype,  u8 labels,
 		   const u32 label[]);
 int nla_get_labels(const struct nlattr *nla, u32 max_labels, u8 *labels,
@@ -123,5 +173,7 @@ int nla_get_via(const struct nlattr *nla, u8 *via_alen, u8 *via_table,
 bool mpls_output_possible(const struct net_device *dev);
 unsigned int mpls_dev_mtu(const struct net_device *dev);
 bool mpls_pkt_too_big(const struct sk_buff *skb, unsigned int mtu);
+void mpls_stats_inc_outucastpkts(struct net_device *dev,
+				 const struct sk_buff *skb);
 
 #endif /* MPLS_INTERNAL_H */
diff --git a/net/mpls/mpls_iptunnel.c b/net/mpls/mpls_iptunnel.c
index 2f7ccd934416..02531284bc49 100644
--- a/net/mpls/mpls_iptunnel.c
+++ b/net/mpls/mpls_iptunnel.c
@@ -48,11 +48,15 @@ static int mpls_xmit(struct sk_buff *skb)
 	struct dst_entry *dst = skb_dst(skb);
 	struct rtable *rt = NULL;
 	struct rt6_info *rt6 = NULL;
+	struct mpls_dev *out_mdev;
 	int err = 0;
 	bool bos;
 	int i;
 	unsigned int ttl;
 
+	/* Find the output device */
+	out_dev = dst->dev;
+
 	/* Obtain the ttl */
 	if (dst->ops->family == AF_INET) {
 		ttl = ip_hdr(skb)->ttl;
@@ -66,8 +70,6 @@ static int mpls_xmit(struct sk_buff *skb)
 
 	skb_orphan(skb);
 
-	/* Find the output device */
-	out_dev = dst->dev;
 	if (!mpls_output_possible(out_dev) ||
 	    !dst->lwtstate || skb_warn_if_lro(skb))
 		goto drop;
@@ -109,6 +111,8 @@ static int mpls_xmit(struct sk_buff *skb)
 		bos = false;
 	}
 
+	mpls_stats_inc_outucastpkts(out_dev, skb);
+
 	if (rt)
 		err = neigh_xmit(NEIGH_ARP_TABLE, out_dev, &rt->rt_gateway,
 				 skb);
@@ -122,6 +126,9 @@ static int mpls_xmit(struct sk_buff *skb)
 	return LWTUNNEL_XMIT_DONE;
 
 drop:
+	out_mdev = out_dev ? mpls_dev_get(out_dev) : NULL;
+	if (out_mdev)
+		MPLS_INC_STATS(out_mdev, tx_errors);
 	kfree_skb(skb);
 	return -EINVAL;
 }
-- 
2.1.4

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox