Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH v4 3/8] MIPS: Octeon: Add a global resource manager.
From: James Hogan @ 2017-11-30 22:53 UTC (permalink / raw)
  To: David Daney
  Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA, ralf-6z/3iImG2C8G8FEW9MqTrA,
	netdev-u79uwXL29TY76Z2rM5mHXA, David S. Miller, Rob Herring,
	Mark Rutland, devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
	Greg Kroah-Hartman, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	Steven J. Hill, devicetree-u79uwXL29TY76Z2rM5mHXA, Andrew Lunn,
	Florian Fainelli, Carlos Munoz
In-Reply-To: <20171129005540.28829-4-david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 14219 bytes --]

On Tue, Nov 28, 2017 at 04:55:35PM -0800, David Daney wrote:
> From: Carlos Munoz <cmunoz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> 
> Add a global resource manager to manage tagged pointers within
> bootmem allocated memory. This is used by various functional
> blocks in the Octeon core like the FPA, Ethernet nexus, etc.
> 
> Signed-off-by: Carlos Munoz <cmunoz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Steven J. Hill <Steven.Hill-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> ---
>  arch/mips/cavium-octeon/Makefile       |   3 +-
>  arch/mips/cavium-octeon/resource-mgr.c | 371 +++++++++++++++++++++++++++++++++
>  arch/mips/include/asm/octeon/octeon.h  |  18 ++
>  3 files changed, 391 insertions(+), 1 deletion(-)
>  create mode 100644 arch/mips/cavium-octeon/resource-mgr.c
> 
> diff --git a/arch/mips/cavium-octeon/Makefile b/arch/mips/cavium-octeon/Makefile
> index 7c02e542959a..0a299ab8719f 100644
> --- a/arch/mips/cavium-octeon/Makefile
> +++ b/arch/mips/cavium-octeon/Makefile
> @@ -9,7 +9,8 @@
>  # Copyright (C) 2005-2009 Cavium Networks
>  #
>  
> -obj-y := cpu.o setup.o octeon-platform.o octeon-irq.o csrc-octeon.o
> +obj-y := cpu.o setup.o octeon-platform.o octeon-irq.o csrc-octeon.o \
> +	 resource-mgr.o

Maybe put that on a separate line like below.

>  obj-y += dma-octeon.o
>  obj-y += octeon-memcpy.o
>  obj-y += executive/
> diff --git a/arch/mips/cavium-octeon/resource-mgr.c b/arch/mips/cavium-octeon/resource-mgr.c
> new file mode 100644
> index 000000000000..ca25fa953402
> --- /dev/null
> +++ b/arch/mips/cavium-octeon/resource-mgr.c
> @@ -0,0 +1,371 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Resource manager for Octeon.
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Copyright (C) 2017 Cavium, Inc.
> + */
> +#include <linux/module.h>
> +
> +#include <asm/octeon/octeon.h>
> +#include <asm/octeon/cvmx-bootmem.h>
> +
> +#define RESOURCE_MGR_BLOCK_NAME		"cvmx-global-resources"
> +#define MAX_RESOURCES			128
> +#define INST_AVAILABLE			-88
> +#define OWNER				0xbadc0de
> +
> +struct global_resource_entry {
> +	struct global_resource_tag tag;
> +	u64 phys_addr;
> +	u64 size;
> +};
> +
> +struct global_resources {
> +#ifdef __LITTLE_ENDIAN_BITFIELD
> +	u32 rlock;
> +	u32 pad;
> +#else
> +	u32 pad;
> +	u32 rlock;
> +#endif
> +	u64 entry_cnt;
> +	struct global_resource_entry resource_entry[];
> +};
> +
> +static struct global_resources *res_mgr_info;
> +
> +
> +/*
> + * The resource manager interacts with software running outside of the
> + * Linux kernel, which necessitates locking to maintain data structure
> + * consistency.  These custom locking functions implement the locking
> + * protocol, and cannot be replaced by kernel locking functions that
> + * may use different in-memory structures.
> + */
> +
> +static void res_mgr_lock(void)
> +{
> +	unsigned int tmp;
> +	u64 lock = (u64)&res_mgr_info->rlock;

presumably this could be a u32 *, avoid the cast to u64, and still work
just fine below.

> +
> +	__asm__ __volatile__(
> +		".set noreorder\n"
> +		"1: ll   %[tmp], 0(%[addr])\n"
> +		"   bnez %[tmp], 1b\n"
> +		"   li   %[tmp], 1\n"

I believe the convention for .S files is for instructions in branch
delay slots to be indented an additional space for readability. Maybe
that would be worthwhile here.

> +		"   sc   %[tmp], 0(%[addr])\n"
> +		"   beqz %[tmp], 1b\n"
> +		"   nop\n"

and here also.

> +		".set reorder\n" :

nit: strictly speaking there's no need for \n on the last line.

> +		[tmp] "=&r"(tmp) :
> +		[addr] "r"(lock) :
> +		"memory");

minor style thing: its far more common to have : at the beginning of the
line rather than the end.

> +}
> +
> +static void res_mgr_unlock(void)
> +{
> +	u64 lock = (u64)&res_mgr_info->rlock;

same again

> +
> +	/* Wait until all resource operations finish before unlocking. */
> +	mb();
> +	__asm__ __volatile__(
> +		"sw $0, 0(%[addr])\n" : :
> +		[addr] "r"(lock) :
> +		"memory");
> +
> +	/* Force a write buffer flush. */
> +	mb();
> +}
> +
> +static int res_mgr_find_resource(struct global_resource_tag tag)
> +{
> +	struct global_resource_entry *res_entry;
> +	int i;
> +
> +	for (i = 0; i < res_mgr_info->entry_cnt; i++) {
> +		res_entry = &res_mgr_info->resource_entry[i];
> +		if (res_entry->tag.lo == tag.lo && res_entry->tag.hi == tag.hi)
> +			return i;
> +	}
> +	return -1;
> +}
> +
> +/**
> + * res_mgr_create_resource - Create a resource.
> + * @tag: Identifies the resource.
> + * @inst_cnt: Number of resource instances to create.
> + *
> + * Returns 0 if the source was created successfully.
> + * Returns <0 for error codes.

Only -1 seems to be returned. Is it worth returning some standard Linux
error codes instead?

> + */
> +int res_mgr_create_resource(struct global_resource_tag tag, int inst_cnt)
> +{
> +	struct global_resource_entry *res_entry;
> +	u64 size;
> +	u64 *res_addr;
> +	int res_index, i, rc = 0;
> +
> +	res_mgr_lock();
> +
> +	/* Make sure resource doesn't already exist. */
> +	res_index = res_mgr_find_resource(tag);
> +	if (res_index >= 0) {
> +		rc = -1;
> +		goto err;
> +	}
> +
> +	if (res_mgr_info->entry_cnt >= MAX_RESOURCES) {
> +		pr_err("Resource max limit reached, not created\n");
> +		rc = -1;
> +		goto err;
> +	}
> +
> +	/*
> +	 * Each instance is kept in an array of u64s. The first array element
> +	 * holds the number of allocated instances.
> +	 */
> +	size = sizeof(u64) * (inst_cnt + 1);
> +	res_addr = cvmx_bootmem_alloc_range(size, CVMX_CACHE_LINE_SIZE, 0, 0);
> +	if (!res_addr) {
> +		pr_err("Failed to allocate resource. not created\n");
> +		rc = -1;
> +		goto err;
> +	}
> +
> +	/* Initialize the newly created resource. */
> +	*res_addr = inst_cnt;
> +	for (i = 1; i < inst_cnt + 1; i++)

or "i <= inst_cnt"?

> +		*(res_addr + i) = INST_AVAILABLE;

Nit: IMO res_addr[i] is marginally more readable

> +
> +	res_index = res_mgr_info->entry_cnt;
> +	res_entry = &res_mgr_info->resource_entry[res_index];
> +	res_entry->tag.lo = tag.lo;
> +	res_entry->tag.hi = tag.hi;

or res_entry->tag = tag;?

> +	res_entry->phys_addr = virt_to_phys(res_addr);
> +	res_entry->size = size;
> +	res_mgr_info->entry_cnt++;
> +
> +err:
> +	res_mgr_unlock();
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL(res_mgr_create_resource);
> +
> +/**
> + * res_mgr_alloc_range - Allocate a range of resource instances.

I don't know how strict kerndoc is on this, but I think it should be
res_mgr_alloc_range() here. Same elsewhere.

> + * @tag: Identifies the resource.
> + * @req_inst: Requested start of instance range to allocate.
> + *	      Range instances are guaranteed to be sequential
> + *	      (-1 for don't care).
> + * @req_cnt: Number of instances to allocate.
> + * @use_last_avail: Set to request the last available instance.
> + * @inst: Updated with the allocated instances.
> + *
> + * Returns 0 if the source was created successfully.
> + * Returns <0 for error codes.
> + */
> +int res_mgr_alloc_range(struct global_resource_tag tag, int req_inst,
> +			int req_cnt, bool use_last_avail, int *inst)
> +{
> +	struct global_resource_entry *res_entry;
> +	int res_index;
> +	u64 *res_addr;
> +	u64 inst_cnt;
> +	int alloc_cnt, i, rc = -1;
> +
> +	/* Start with no instances allocated. */
> +	for (i = 0; i < req_cnt; i++)
> +		inst[i] = INST_AVAILABLE;
> +
> +	res_mgr_lock();
> +
> +	/* Find the resource. */
> +	res_index = res_mgr_find_resource(tag);
> +	if (res_index < 0) {
> +		pr_err("Resource not found, can't allocate instance\n");
> +		goto err;
> +	}
> +
> +	/* Get resource data. */
> +	res_entry = &res_mgr_info->resource_entry[res_index];
> +	res_addr = phys_to_virt(res_entry->phys_addr);
> +	inst_cnt = *res_addr;
> +
> +	/* Allocate the requested instances. */
> +	if (req_inst >= 0) {
> +		/* Specific instance range requested. */
> +		if (req_inst + req_cnt >= inst_cnt) {
> +			pr_err("Requested instance out of range\n");
> +			goto err;
> +		}
> +
> +		for (i = 0; i < req_cnt; i++) {
> +			if (*(res_addr + req_inst + 1 + i) == INST_AVAILABLE)
> +				inst[i] = req_inst + i;
> +			else {

braces on all branches if on any.

> +				inst[0] = INST_AVAILABLE;
> +				break;
> +			}
> +		}
> +	} else if (use_last_avail) {
> +		/* Last available instance requested. */
> +		alloc_cnt = 0;
> +		for (i = inst_cnt; i > 0; i--) {
> +			if (*(res_addr + i) == INST_AVAILABLE) {
> +				/*
> +				 * Instance off by 1 (first element holds the
> +				 * count).
> +				 */
> +				inst[alloc_cnt] = i - 1;
> +
> +				alloc_cnt++;
> +				if (alloc_cnt == req_cnt)
> +					break;
> +			}
> +		}
> +
> +		if (i == 0)
> +			inst[0] = INST_AVAILABLE;
> +	} else {
> +		/* Next available instance requested. */
> +		alloc_cnt = 0;
> +		for (i = 1; i <= inst_cnt; i++) {
> +			if (*(res_addr + i) == INST_AVAILABLE) {
> +				/*
> +				 * Instance off by 1 (first element holds the
> +				 * count).
> +				 */
> +				inst[alloc_cnt] = i - 1;
> +
> +				alloc_cnt++;
> +				if (alloc_cnt == req_cnt)
> +					break;
> +			}
> +		}
> +
> +		if (i > inst_cnt)
> +			inst[0] = INST_AVAILABLE;
> +	}
> +
> +	if (inst[0] != INST_AVAILABLE) {
> +		for (i = 0; i < req_cnt; i++)
> +			*(res_addr + inst[i] + 1) = OWNER;
> +		rc = 0;
> +	}
> +
> +err:
> +	res_mgr_unlock();
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL(res_mgr_alloc_range);
> +
> +/**
> + * res_mgr_alloc - Allocate a resource instance.
> + * @tag: Identifies the resource.
> + * @req_inst: Requested instance to allocate (-1 for don't care).
> + * @use_last_avail: Set to request the last available instance.
> + *
> + * Returns: Allocated resource instance if successful.
> + * Returns <0 for error codes.
> + */
> +int res_mgr_alloc(struct global_resource_tag tag, int req_inst, bool use_last_avail)
> +{
> +	int inst, rc;
> +
> +	rc = res_mgr_alloc_range(tag, req_inst, 1, use_last_avail, &inst);
> +	if (!rc)
> +		return inst;
> +	return rc;
> +}
> +EXPORT_SYMBOL(res_mgr_alloc);
> +
> +/**
> + * res_mgr_free_range - Free a resource instance range.
> + * @tag: Identifies the resource.
> + * @req_inst: Requested instance to free.

the parameter is called inst.

Other than these minor / style comments, it doesn't look unreasonable to
me.

Cheers
James

> + * @req_cnt: Number of instances to free.
> + */
> +void res_mgr_free_range(struct global_resource_tag tag, const int *inst, int req_cnt)
> +{
> +	struct global_resource_entry *res_entry;
> +	int res_index, i;
> +	u64 *res_addr;
> +
> +	res_mgr_lock();
> +
> +	/* Find the resource. */
> +	res_index = res_mgr_find_resource(tag);
> +	if (res_index < 0) {
> +		pr_err("Resource not found, can't free instance\n");
> +		goto err;
> +	}
> +
> +	/* Get the resource data. */
> +	res_entry = &res_mgr_info->resource_entry[res_index];
> +	res_addr = phys_to_virt(res_entry->phys_addr);
> +
> +	/* Free the resource instances. */
> +	for (i = 0; i < req_cnt; i++) {
> +		/* Instance off by 1 (first element holds the count). */
> +		*(res_addr + inst[i] + 1) = INST_AVAILABLE;
> +	}
> +
> +err:
> +	res_mgr_unlock();
> +}
> +EXPORT_SYMBOL(res_mgr_free_range);
> +
> +/**
> + * res_mgr_free - Free a resource instance.
> + * @tag: Identifies the resource.
> + * @req_inst: Requested instance to free.
> + */
> +void res_mgr_free(struct global_resource_tag tag, int inst)
> +{
> +	res_mgr_free_range(tag, &inst, 1);
> +}
> +EXPORT_SYMBOL(res_mgr_free);
> +
> +static int __init res_mgr_init(void)
> +{
> +	struct cvmx_bootmem_named_block_desc *block;
> +	int block_size;
> +	u64 addr;
> +
> +	cvmx_bootmem_lock();
> +
> +	/* Search for the resource manager data in boot memory. */
> +	block = cvmx_bootmem_phy_named_block_find(RESOURCE_MGR_BLOCK_NAME, CVMX_BOOTMEM_FLAG_NO_LOCKING);
> +	if (block) {
> +		/* Found. */
> +		res_mgr_info = phys_to_virt(block->base_addr);
> +	} else {
> +		/* Create it. */
> +		block_size = sizeof(struct global_resources) +
> +			sizeof(struct global_resource_entry) * MAX_RESOURCES;
> +		addr = cvmx_bootmem_phy_named_block_alloc(block_size, 0, 0,
> +				CVMX_CACHE_LINE_SIZE, RESOURCE_MGR_BLOCK_NAME,
> +				CVMX_BOOTMEM_FLAG_NO_LOCKING);
> +		if (!addr) {
> +			pr_err("Failed to allocate name block %s\n",
> +			       RESOURCE_MGR_BLOCK_NAME);
> +		} else {
> +			res_mgr_info = phys_to_virt(addr);
> +			memset(res_mgr_info, 0, block_size);
> +		}
> +	}
> +
> +	cvmx_bootmem_unlock();
> +
> +	return 0;
> +}
> +device_initcall(res_mgr_init);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Cavium, Inc. Octeon resource manager");
> diff --git a/arch/mips/include/asm/octeon/octeon.h b/arch/mips/include/asm/octeon/octeon.h
> index 92a17d67c1fa..0411efdb465c 100644
> --- a/arch/mips/include/asm/octeon/octeon.h
> +++ b/arch/mips/include/asm/octeon/octeon.h
> @@ -346,6 +346,24 @@ void octeon_mult_restore3_end(void);
>  void octeon_mult_restore2(void);
>  void octeon_mult_restore2_end(void);
>  
> +/*
> + * This definition must be kept in sync with the one in
> + * cvmx-global-resources.c
> + */
> +struct global_resource_tag {
> +	uint64_t lo;
> +	uint64_t hi;
> +};
> +
> +void res_mgr_free(struct global_resource_tag tag, int inst);
> +void res_mgr_free_range(struct global_resource_tag tag, const int *inst,
> +			int req_cnt);
> +int res_mgr_alloc(struct global_resource_tag tag, int req_inst,
> +		  bool use_last_avail);
> +int res_mgr_alloc_range(struct global_resource_tag tag, int req_inst,
> +			int req_cnt, bool use_last_avail, int *inst);
> +int res_mgr_create_resource(struct global_resource_tag tag, int inst_cnt);
> +
>  /**
>   * Read a 32bit value from the Octeon NPI register space
>   *
> -- 
> 2.14.3
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()
From: Cong Wang @ 2017-11-30 22:53 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang, Jiri Pirko, Jamal Hadi Salim

tcfm_dev always points to the correct netdev and we already
hold a refcnt, so no need to use ifindex to lookup again.

If we would support moving target netdev across netns, using
pointer would be better than ifindex.

Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/tc_act/tc_mirred.h | 1 -
 net/sched/act_mirred.c         | 3 +--
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
index 21d253c9a8c6..b2dbbfaefd22 100644
--- a/include/net/tc_act/tc_mirred.h
+++ b/include/net/tc_act/tc_mirred.h
@@ -11,7 +11,6 @@ struct tcf_mirred {
 	int			tcfm_ifindex;
 	bool			tcfm_mac_header_xmit;
 	struct net_device __rcu	*tcfm_dev;
-	struct net		*net;
 	struct list_head	tcfm_list;
 };
 #define to_mirred(a) ((struct tcf_mirred *)a)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 8b3e59388480..fe6489f9c3cf 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -140,7 +140,6 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	m->tcfm_eaction = parm->eaction;
 	if (dev != NULL) {
 		m->tcfm_ifindex = parm->ifindex;
-		m->net = net;
 		if (ret != ACT_P_CREATED)
 			dev_put(rcu_dereference_protected(m->tcfm_dev, 1));
 		dev_hold(dev);
@@ -318,7 +317,7 @@ static struct net_device *tcf_mirred_get_dev(const struct tc_action *a)
 {
 	struct tcf_mirred *m = to_mirred(a);
 
-	return __dev_get_by_index(m->net, m->tcfm_ifindex);
+	return rtnl_dereference(m->tcfm_dev);
 }
 
 static struct tc_action_ops act_mirred_ops = {
-- 
2.13.0

^ permalink raw reply related

* [Patch net-next] act_mirred: get rid of mirred_list_lock spinlock
From: Cong Wang @ 2017-11-30 22:53 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang, Jiri Pirko, Jamal Hadi Salim
In-Reply-To: <20171130225335.6957-1-xiyou.wangcong@gmail.com>

TC actions are no longer freed in RCU callbacks and we should
always have RTNL lock, so this spinlock is no longer needed.

Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/act_mirred.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index fe6489f9c3cf..2c51952bf2d4 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -29,7 +29,6 @@
 #include <net/tc_act/tc_mirred.h>
 
 static LIST_HEAD(mirred_list);
-static DEFINE_SPINLOCK(mirred_list_lock);
 
 static bool tcf_mirred_is_act_redirect(int action)
 {
@@ -55,13 +54,10 @@ static void tcf_mirred_release(struct tc_action *a, int bind)
 	struct tcf_mirred *m = to_mirred(a);
 	struct net_device *dev;
 
-	/* We could be called either in a RCU callback or with RTNL lock held. */
-	spin_lock_bh(&mirred_list_lock);
 	list_del(&m->tcfm_list);
 	dev = rcu_dereference_protected(m->tcfm_dev, 1);
 	if (dev)
 		dev_put(dev);
-	spin_unlock_bh(&mirred_list_lock);
 }
 
 static const struct nla_policy mirred_policy[TCA_MIRRED_MAX + 1] = {
@@ -148,9 +144,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 	}
 
 	if (ret == ACT_P_CREATED) {
-		spin_lock_bh(&mirred_list_lock);
 		list_add(&m->tcfm_list, &mirred_list);
-		spin_unlock_bh(&mirred_list_lock);
 		tcf_idr_insert(tn, *a);
 	}
 
@@ -293,7 +287,6 @@ static int mirred_device_event(struct notifier_block *unused,
 
 	ASSERT_RTNL();
 	if (event == NETDEV_UNREGISTER) {
-		spin_lock_bh(&mirred_list_lock);
 		list_for_each_entry(m, &mirred_list, tcfm_list) {
 			if (rcu_access_pointer(m->tcfm_dev) == dev) {
 				dev_put(dev);
@@ -303,7 +296,6 @@ static int mirred_device_event(struct notifier_block *unused,
 				RCU_INIT_POINTER(m->tcfm_dev, NULL);
 			}
 		}
-		spin_unlock_bh(&mirred_list_lock);
 	}
 
 	return NOTIFY_DONE;
-- 
2.13.0

^ permalink raw reply related

* [PATCH net-next resubmit 1/2] net: phy: core: remove now uneeded disabling of interrupts
From: Heiner Kallweit @ 2017-11-30 22:55 UTC (permalink / raw)
  To: Florian Fainelli, Andrew Lunn, David Miller
  Cc: netdev@vger.kernel.org, Ard Biesheuvel

After commits c974bdbc3e "net: phy: Use threaded IRQ, to allow IRQ from
sleeping devices" and 664fcf123a30 "net: phy: Threaded interrupts allow
some simplification" all relevant code pieces run in process context
anyway and I don't think we need the disabling of interrupts any longer.

Interestingly enough, latter commit already removed the comment
explaining why interrupts need to be temporarily disabled.

On my system phy interrupt mode works fine with this patch.
However I may miss something, especially in the context of shared phy
interrupts, therefore I'd appreciate if more people could test this.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/net/phy/phy.c | 26 ++------------------------
 include/linux/phy.h   |  1 -
 2 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2b1e67bc1..b3784c9a2 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -629,9 +629,6 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	if (PHY_HALTED == phydev->state)
 		return IRQ_NONE;		/* It can't be ours.  */
 
-	disable_irq_nosync(irq);
-	atomic_inc(&phydev->irq_disable);
-
 	phy_change(phydev);
 
 	return IRQ_HANDLED;
@@ -689,7 +686,6 @@ static int phy_disable_interrupts(struct phy_device *phydev)
  */
 int phy_start_interrupts(struct phy_device *phydev)
 {
-	atomic_set(&phydev->irq_disable, 0);
 	if (request_threaded_irq(phydev->irq, NULL, phy_interrupt,
 				 IRQF_ONESHOT | IRQF_SHARED,
 				 phydev_name(phydev), phydev) < 0) {
@@ -716,13 +712,6 @@ int phy_stop_interrupts(struct phy_device *phydev)
 
 	free_irq(phydev->irq, phydev);
 
-	/* If work indeed has been cancelled, disable_irq() will have
-	 * been left unbalanced from phy_interrupt() and enable_irq()
-	 * has to be called so that other devices on the line work.
-	 */
-	while (atomic_dec_return(&phydev->irq_disable) >= 0)
-		enable_irq(phydev->irq);
-
 	return err;
 }
 EXPORT_SYMBOL(phy_stop_interrupts);
@@ -736,7 +725,7 @@ void phy_change(struct phy_device *phydev)
 	if (phy_interrupt_is_valid(phydev)) {
 		if (phydev->drv->did_interrupt &&
 		    !phydev->drv->did_interrupt(phydev))
-			goto ignore;
+			return;
 
 		if (phy_disable_interrupts(phydev))
 			goto phy_err;
@@ -748,27 +737,16 @@ void phy_change(struct phy_device *phydev)
 	mutex_unlock(&phydev->lock);
 
 	if (phy_interrupt_is_valid(phydev)) {
-		atomic_dec(&phydev->irq_disable);
-		enable_irq(phydev->irq);
-
 		/* Reenable interrupts */
 		if (PHY_HALTED != phydev->state &&
 		    phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED))
-			goto irq_enable_err;
+			goto phy_err;
 	}
 
 	/* reschedule state queue work to run as soon as possible */
 	phy_trigger_machine(phydev, true);
 	return;
 
-ignore:
-	atomic_dec(&phydev->irq_disable);
-	enable_irq(phydev->irq);
-	return;
-
-irq_enable_err:
-	disable_irq(phydev->irq);
-	atomic_inc(&phydev->irq_disable);
 phy_err:
 	phy_error(phydev);
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index dc82a07cb..8a87e441f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -468,7 +468,6 @@ struct phy_device {
 	/* Interrupt and Polling infrastructure */
 	struct work_struct phy_queue;
 	struct delayed_work state_queue;
-	atomic_t irq_disable;
 
 	struct mutex lock;
 
-- 
2.15.0

^ permalink raw reply related

* Re: [PATCH v4 2/8] MIPS: Octeon: Enable LMTDMA/LMTST operations.
From: James Hogan @ 2017-11-30 22:56 UTC (permalink / raw)
  To: David Daney
  Cc: David Daney, linux-mips, ralf, netdev, David S. Miller,
	Rob Herring, Mark Rutland, devel, Greg Kroah-Hartman,
	linux-kernel, Steven J. Hill, devicetree, Andrew Lunn,
	Florian Fainelli, Carlos Munoz
In-Reply-To: <54c83e6b-35e2-be38-e4f1-87eb420938cb@caviumnetworks.com>

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

On Thu, Nov 30, 2017 at 01:49:43PM -0800, David Daney wrote:
> On 11/30/2017 01:36 PM, James Hogan wrote:
> > On Tue, Nov 28, 2017 at 04:55:34PM -0800, David Daney wrote:
> >> Signed-off-by: Carlos Munoz <cmunoz@cavium.com>
> >> Signed-off-by: Steven J. Hill <Steven.Hill@cavium.com>
> >> Signed-off-by: David Daney <david.daney@cavium.com>
> >> ---
> >>   arch/mips/cavium-octeon/setup.c       |  6 ++++++
> >>   arch/mips/include/asm/octeon/octeon.h | 12 ++++++++++--
> >>   2 files changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c
> >> index a8034d0dcade..99e6a68bc652 100644
> >> --- a/arch/mips/cavium-octeon/setup.c
> >> +++ b/arch/mips/cavium-octeon/setup.c
> >> @@ -609,6 +609,12 @@ void octeon_user_io_init(void)
> >>   #else
> >>   	cvmmemctl.s.cvmsegenak = 0;
> >>   #endif
> >> +	if (OCTEON_IS_OCTEON3()) {
> >> +		/* Enable LMTDMA */
> >> +		cvmmemctl.s.lmtena = 1;
> >> +		/* Scratch line to use for LMT operation */
> >> +		cvmmemctl.s.lmtline = 2;
> > 
> > Out of curiosity, is there significance to the value 2 and associated
> > virtual address 0xffffffffffff8100, or is it pretty arbitrary?
> 
> Yes, there is significance.
> 
> CPU local memory starts at 0xffffffffffff8000, each line is 0x80 bytes. 
> so the 2nd line starts at 0xffffffffffff8100

What I mean is, why is 2 chosen instead of any other value?

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* [PATCH net-next resubmit 2/2] net: phy: core: don't disable device interrupts in phy_change
From: Heiner Kallweit @ 2017-11-30 22:57 UTC (permalink / raw)
  To: Florian Fainelli, Andrew Lunn, David Miller
  Cc: netdev@vger.kernel.org, Ard Biesheuvel
In-Reply-To: <ba11e1ae-dc21-e036-f918-1ed44c92a424@gmail.com>

If state is not PHY_HALTED I see no need to temporarily disable
interrupts on the device. As long as the current interrupt isn't acked
on the device no new interrupt can happen anyway.

In addition remove a unneeded enabling of interrupts in the state
machine when handling state PHY_CHANGELINK.

Tested on a Odroid-C2 with RTL8211F phy in interrupt mode.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index b3784c9a2..4a11de8cb 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -727,8 +727,9 @@ void phy_change(struct phy_device *phydev)
 		    !phydev->drv->did_interrupt(phydev))
 			return;
 
-		if (phy_disable_interrupts(phydev))
-			goto phy_err;
+		if (phydev->state == PHY_HALTED)
+			if (phy_disable_interrupts(phydev))
+				goto phy_err;
 	}
 
 	mutex_lock(&phydev->lock);
@@ -736,15 +737,11 @@ void phy_change(struct phy_device *phydev)
 		phydev->state = PHY_CHANGELINK;
 	mutex_unlock(&phydev->lock);
 
-	if (phy_interrupt_is_valid(phydev)) {
-		/* Reenable interrupts */
-		if (PHY_HALTED != phydev->state &&
-		    phy_config_interrupt(phydev, PHY_INTERRUPT_ENABLED))
-			goto phy_err;
-	}
-
 	/* reschedule state queue work to run as soon as possible */
 	phy_trigger_machine(phydev, true);
+
+	if (phy_interrupt_is_valid(phydev) && phy_clear_interrupt(phydev))
+		goto phy_err;
 	return;
 
 phy_err:
@@ -984,10 +981,6 @@ void phy_state_machine(struct work_struct *work)
 			phydev->state = PHY_NOLINK;
 			phy_link_down(phydev, true);
 		}
-
-		if (phy_interrupt_is_valid(phydev))
-			err = phy_config_interrupt(phydev,
-						   PHY_INTERRUPT_ENABLED);
 		break;
 	case PHY_HALTED:
 		if (phydev->link) {
-- 
2.15.0

^ permalink raw reply related

* Re: [PATCH net-next 7/8] netdevsim: add SR-IOV functionality
From: Jakub Kicinski @ 2017-11-30 22:57 UTC (permalink / raw)
  To: netdev
  Cc: davem, daniel, alexei.starovoitov, jiri, oss-drivers, Phil Sutter,
	Sabrina Dubroca
In-Reply-To: <20171130222853.29395-8-jakub.kicinski@netronome.com>

On Thu, 30 Nov 2017 14:28:52 -0800, Jakub Kicinski wrote:
> +static void nsim_free(struct net_device *dev)
> +{
> +	struct netdevsim *ns = netdev_priv(dev);
> +
> +	device_unregister(&ns->dev);
>  }

self-nack, I need to free the device from release handler... sorry...

^ permalink raw reply

* Re: [PATCH v4 2/8] MIPS: Octeon: Enable LMTDMA/LMTST operations.
From: David Daney @ 2017-11-30 23:09 UTC (permalink / raw)
  To: James Hogan
  Cc: David Daney, linux-mips-6z/3iImG2C8G8FEW9MqTrA,
	ralf-6z/3iImG2C8G8FEW9MqTrA, netdev-u79uwXL29TY76Z2rM5mHXA,
	David S. Miller, Rob Herring, Mark Rutland,
	devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, Greg Kroah-Hartman,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steven J. Hill,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Andrew Lunn, Florian Fainelli,
	Carlos Munoz
In-Reply-To: <20171130225614.GJ27409-4bYivNCBEGSP4qXr0kR+DFHK5/nzsB32@public.gmane.org>

On 11/30/2017 02:56 PM, James Hogan wrote:
> On Thu, Nov 30, 2017 at 01:49:43PM -0800, David Daney wrote:
>> On 11/30/2017 01:36 PM, James Hogan wrote:
>>> On Tue, Nov 28, 2017 at 04:55:34PM -0800, David Daney wrote:
>>>> Signed-off-by: Carlos Munoz <cmunoz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>>> Signed-off-by: Steven J. Hill <Steven.Hill-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>>> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>    arch/mips/cavium-octeon/setup.c       |  6 ++++++
>>>>    arch/mips/include/asm/octeon/octeon.h | 12 ++++++++++--
>>>>    2 files changed, 16 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c
>>>> index a8034d0dcade..99e6a68bc652 100644
>>>> --- a/arch/mips/cavium-octeon/setup.c
>>>> +++ b/arch/mips/cavium-octeon/setup.c
>>>> @@ -609,6 +609,12 @@ void octeon_user_io_init(void)
>>>>    #else
>>>>    	cvmmemctl.s.cvmsegenak = 0;
>>>>    #endif
>>>> +	if (OCTEON_IS_OCTEON3()) {
>>>> +		/* Enable LMTDMA */
>>>> +		cvmmemctl.s.lmtena = 1;
>>>> +		/* Scratch line to use for LMT operation */
>>>> +		cvmmemctl.s.lmtline = 2;
>>>
>>> Out of curiosity, is there significance to the value 2 and associated
>>> virtual address 0xffffffffffff8100, or is it pretty arbitrary?
>>
>> Yes, there is significance.
>>
>> CPU local memory starts at 0xffffffffffff8000, each line is 0x80 bytes.
>> so the 2nd line starts at 0xffffffffffff8100
> 
> What I mean is, why is 2 chosen instead of any other value?

That is explained in the change log of patch 5/8:


     1st 128-bytes: Use by IOBDMA
     2nd 128-bytes: Reserved by kernel for scratch/TLS emulation.
     3rd 128-bytes: OCTEON-III LMTLINE

> 
> Cheers
> James
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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: [PATCH v4 2/8] MIPS: Octeon: Enable LMTDMA/LMTST operations.
From: James Hogan @ 2017-11-30 23:12 UTC (permalink / raw)
  To: David Daney
  Cc: David Daney, linux-mips-6z/3iImG2C8G8FEW9MqTrA,
	ralf-6z/3iImG2C8G8FEW9MqTrA, netdev-u79uwXL29TY76Z2rM5mHXA,
	David S. Miller, Rob Herring, Mark Rutland,
	devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b, Greg Kroah-Hartman,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Steven J. Hill,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Andrew Lunn, Florian Fainelli,
	Carlos Munoz
In-Reply-To: <c90ac3a5-7230-38ff-691a-3d94a25702cd-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 2030 bytes --]

On Thu, Nov 30, 2017 at 03:09:33PM -0800, David Daney wrote:
> On 11/30/2017 02:56 PM, James Hogan wrote:
> > On Thu, Nov 30, 2017 at 01:49:43PM -0800, David Daney wrote:
> >> On 11/30/2017 01:36 PM, James Hogan wrote:
> >>> On Tue, Nov 28, 2017 at 04:55:34PM -0800, David Daney wrote:
> >>>> Signed-off-by: Carlos Munoz <cmunoz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> >>>> Signed-off-by: Steven J. Hill <Steven.Hill-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> >>>> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> >>>> ---
> >>>>    arch/mips/cavium-octeon/setup.c       |  6 ++++++
> >>>>    arch/mips/include/asm/octeon/octeon.h | 12 ++++++++++--
> >>>>    2 files changed, 16 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c
> >>>> index a8034d0dcade..99e6a68bc652 100644
> >>>> --- a/arch/mips/cavium-octeon/setup.c
> >>>> +++ b/arch/mips/cavium-octeon/setup.c
> >>>> @@ -609,6 +609,12 @@ void octeon_user_io_init(void)
> >>>>    #else
> >>>>    	cvmmemctl.s.cvmsegenak = 0;
> >>>>    #endif
> >>>> +	if (OCTEON_IS_OCTEON3()) {
> >>>> +		/* Enable LMTDMA */
> >>>> +		cvmmemctl.s.lmtena = 1;
> >>>> +		/* Scratch line to use for LMT operation */
> >>>> +		cvmmemctl.s.lmtline = 2;
> >>>
> >>> Out of curiosity, is there significance to the value 2 and associated
> >>> virtual address 0xffffffffffff8100, or is it pretty arbitrary?
> >>
> >> Yes, there is significance.
> >>
> >> CPU local memory starts at 0xffffffffffff8000, each line is 0x80 bytes.
> >> so the 2nd line starts at 0xffffffffffff8100
> > 
> > What I mean is, why is 2 chosen instead of any other value?
> 
> That is explained in the change log of patch 5/8:
> 
> 
>      1st 128-bytes: Use by IOBDMA
>      2nd 128-bytes: Reserved by kernel for scratch/TLS emulation.
>      3rd 128-bytes: OCTEON-III LMTLINE

Ah yes. Perhaps it deserves a brief comment in the code, or even an
enum.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply

* Re: [Patch net-next] act_mirred: get rid of mirred_list_lock spinlock
From: Eric Dumazet @ 2017-11-30 23:12 UTC (permalink / raw)
  To: Cong Wang, netdev; +Cc: Jiri Pirko, Jamal Hadi Salim
In-Reply-To: <20171130225335.6957-2-xiyou.wangcong@gmail.com>

On Thu, 2017-11-30 at 14:53 -0800, Cong Wang wrote:
> TC actions are no longer freed in RCU callbacks and we should
> always have RTNL lock, so this spinlock is no longer needed.
> 
> Cc: Jiri Pirko <jiri@mellanox.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/sched/act_mirred.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index fe6489f9c3cf..2c51952bf2d4 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -29,7 +29,6 @@
>  #include <net/tc_act/tc_mirred.h>
>  
>  static LIST_HEAD(mirred_list);
> -static DEFINE_SPINLOCK(mirred_list_lock);
>  
>  static bool tcf_mirred_is_act_redirect(int action)
>  {
> @@ -55,13 +54,10 @@ static void tcf_mirred_release(struct tc_action
> *a, int bind)
>  	struct tcf_mirred *m = to_mirred(a);
>  	struct net_device *dev;
>  
> -	/* We could be called either in a RCU callback or with RTNL
> lock held. */
> -	spin_lock_bh(&mirred_list_lock);
>  	list_del(&m->tcfm_list);
>  	dev = rcu_dereference_protected(m->tcfm_dev, 1);

If RTNL is held at this point, I suggest to use
rtnl_dereference() instead of rcu_dereference_protected() to get proper
lockdep coverage.


>  	if (dev)
>  		dev_put(dev);
> -	spin_unlock_bh(&mirred_list_lock);
>  }
>  

^ permalink raw reply

* [PATCH net-next 0/4] tcp: Add a 2nd listener hashtable (port+addr)
From: Martin KaFai Lau @ 2017-11-30 23:23 UTC (permalink / raw)
  To: netdev; +Cc: David S . Miller, Eric Dumazet, Kernel Team

This patch set adds a 2nd listener hashtable.  It is to resolve
the performance issue when a process is listening at many IP
addresses with the same port (e.g. [IP1]:443, [IP2]:443... [IPN]:443)

Martin KaFai Lau (4):
  inet: Add a count to struct inet_listen_hashbucket
  udp: Move udp[46]_portaddr_hash() to net/ip[v6].h
  inet: Add a 2nd listener hashtable (port+addr)
  tcp: Enable 2nd listener hashtable in TCP

 include/net/inet_connection_sock.h |   2 +
 include/net/inet_hashtables.h      |  16 ++++
 include/net/ip.h                   |   9 ++
 include/net/ipv6.h                 |  17 ++++
 net/ipv4/inet_hashtables.c         | 173 +++++++++++++++++++++++++++++++++++--
 net/ipv4/tcp.c                     |   3 +
 net/ipv4/udp.c                     |  22 ++---
 net/ipv6/inet6_hashtables.c        |  66 ++++++++++++++
 net/ipv6/udp.c                     |  32 ++-----
 9 files changed, 294 insertions(+), 46 deletions(-)

-- 
2.9.5

^ permalink raw reply

* [PATCH net-next 1/4] inet: Add a count to struct inet_listen_hashbucket
From: Martin KaFai Lau @ 2017-11-30 23:23 UTC (permalink / raw)
  To: netdev; +Cc: David S . Miller, Eric Dumazet, Kernel Team
In-Reply-To: <20171130232327.561108-1-kafai@fb.com>

This patch adds a count to the 'struct inet_listen_hashbucket'.
It counts how many sk is hashed to a bucket.  It will be
used to decide if the (to-be-added) portaddr listener's hashtable
should be used during inet[6]_lookup_listener().

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 include/net/inet_hashtables.h |  1 +
 net/ipv4/inet_hashtables.c    | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index 2dbbbff5e1e3..4cce516c41ac 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -111,6 +111,7 @@ struct inet_bind_hashbucket {
  */
 struct inet_listen_hashbucket {
 	spinlock_t		lock;
+	unsigned int		count;
 	struct hlist_head	head;
 };
 
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 427b705d7c64..80cfd3fa21ca 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -476,6 +476,7 @@ int __inet_hash(struct sock *sk, struct sock *osk)
 		hlist_add_tail_rcu(&sk->sk_node, &ilb->head);
 	else
 		hlist_add_head_rcu(&sk->sk_node, &ilb->head);
+	ilb->count++;
 	sock_set_flag(sk, SOCK_RCU_FREE);
 	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
 unlock:
@@ -502,6 +503,7 @@ EXPORT_SYMBOL_GPL(inet_hash);
 void inet_unhash(struct sock *sk)
 {
 	struct inet_hashinfo *hashinfo = sk->sk_prot->h.hashinfo;
+	struct inet_listen_hashbucket *ilb;
 	spinlock_t *lock;
 	bool listener = false;
 	int done;
@@ -510,7 +512,8 @@ void inet_unhash(struct sock *sk)
 		return;
 
 	if (sk->sk_state == TCP_LISTEN) {
-		lock = &hashinfo->listening_hash[inet_sk_listen_hashfn(sk)].lock;
+		ilb = &hashinfo->listening_hash[inet_sk_listen_hashfn(sk)];
+		lock = &ilb->lock;
 		listener = true;
 	} else {
 		lock = inet_ehash_lockp(hashinfo, sk->sk_hash);
@@ -522,8 +525,11 @@ void inet_unhash(struct sock *sk)
 		done = __sk_del_node_init(sk);
 	else
 		done = __sk_nulls_del_node_init_rcu(sk);
-	if (done)
+	if (done) {
+		if (listener)
+			ilb->count--;
 		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
+	}
 	spin_unlock_bh(lock);
 }
 EXPORT_SYMBOL_GPL(inet_unhash);
@@ -658,6 +664,7 @@ void inet_hashinfo_init(struct inet_hashinfo *h)
 	for (i = 0; i < INET_LHTABLE_SIZE; i++) {
 		spin_lock_init(&h->listening_hash[i].lock);
 		INIT_HLIST_HEAD(&h->listening_hash[i].head);
+		h->listening_hash[i].count = 0;
 	}
 }
 EXPORT_SYMBOL_GPL(inet_hashinfo_init);
-- 
2.9.5

^ permalink raw reply related

* [PATCH net-next 4/4] tcp: Enable 2nd listener hashtable in TCP
From: Martin KaFai Lau @ 2017-11-30 23:23 UTC (permalink / raw)
  To: netdev; +Cc: David S . Miller, Eric Dumazet, Kernel Team
In-Reply-To: <20171130232327.561108-1-kafai@fb.com>

Enable the second listener hashtable in TCP.
The scale is the same as UDP which is one slot per 2MB.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 net/ipv4/tcp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index bf97317e6c97..180311636023 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3577,6 +3577,9 @@ void __init tcp_init(void)
 	percpu_counter_init(&tcp_sockets_allocated, 0, GFP_KERNEL);
 	percpu_counter_init(&tcp_orphan_count, 0, GFP_KERNEL);
 	inet_hashinfo_init(&tcp_hashinfo);
+	inet_hashinfo2_init(&tcp_hashinfo, "tcp_listen_portaddr_hash",
+			    thash_entries, 21,  /* one slot per 2 MB*/
+			    0, 64 * 1024);
 	tcp_hashinfo.bind_bucket_cachep =
 		kmem_cache_create("tcp_bind_bucket",
 				  sizeof(struct inet_bind_bucket), 0,
-- 
2.9.5

^ permalink raw reply related

* [PATCH net-next 3/4] inet: Add a 2nd listener hashtable (port+addr)
From: Martin KaFai Lau @ 2017-11-30 23:23 UTC (permalink / raw)
  To: netdev; +Cc: David S . Miller, Eric Dumazet, Kernel Team
In-Reply-To: <20171130232327.561108-1-kafai@fb.com>

The current listener hashtable is hashed by port only.
When a process is listening at many IP addresses with the same port (e.g.
[IP1]:443, [IP2]:443... [IPN]:443), the inet[6]_lookup_listener()
performance is degraded to a link list.  It is prone to syn attack.

UDP had a similar issue and a second hashtable was added to resolve it.

This patch adds a second hashtable for the listener's sockets.
The second hashtable is hashed by port and address.

It cannot reuse the existing skc_portaddr_node which is shared
with skc_bind_node.  TCP listener needs to use skc_bind_node.
Instead, this patch adds a hlist_node 'icsk_listen_portaddr_node' to
the inet_connection_sock which the listener (like TCP) also belongs to.

The new portaddr hashtable may need two lookup (First by IP:PORT.
Second by INADDR_ANY:PORT if the IP:PORT is a not found).   Hence,
it implements a similar cut off as UDP such that it will only consult the
new portaddr hashtable if the current port-only hashtable has >10
sk in the link-list.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 include/net/inet_connection_sock.h |   2 +
 include/net/inet_hashtables.h      |  15 ++++
 net/ipv4/inet_hashtables.c         | 168 +++++++++++++++++++++++++++++++++++--
 net/ipv6/inet6_hashtables.c        |  66 +++++++++++++++
 4 files changed, 242 insertions(+), 9 deletions(-)

diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index 0358745ea059..8e1bf9ae4a5e 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h
@@ -77,6 +77,7 @@ struct inet_connection_sock_af_ops {
  * @icsk_af_ops		   Operations which are AF_INET{4,6} specific
  * @icsk_ulp_ops	   Pluggable ULP control hook
  * @icsk_ulp_data	   ULP private data
+ * @icsk_listen_portaddr_node	hash to the portaddr listener hashtable
  * @icsk_ca_state:	   Congestion control state
  * @icsk_retransmits:	   Number of unrecovered [RTO] timeouts
  * @icsk_pending:	   Scheduled timer event
@@ -101,6 +102,7 @@ struct inet_connection_sock {
 	const struct inet_connection_sock_af_ops *icsk_af_ops;
 	const struct tcp_ulp_ops  *icsk_ulp_ops;
 	void			  *icsk_ulp_data;
+	struct hlist_node         icsk_listen_portaddr_node;
 	unsigned int		  (*icsk_sync_mss)(struct sock *sk, u32 pmtu);
 	__u8			  icsk_ca_state:6,
 				  icsk_ca_setsockopt:1,
diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index 4cce516c41ac..ebce55d694e7 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -152,8 +152,19 @@ struct inet_hashinfo {
 	 */
 	struct inet_listen_hashbucket	listening_hash[INET_LHTABLE_SIZE]
 					____cacheline_aligned_in_smp;
+	struct inet_listen_hashbucket	*lhash2;
+	unsigned int			lhash2_mask;
 };
 
+#define inet_lhash2_for_each_icsk_rcu(__icsk, list) \
+	hlist_for_each_entry_rcu(__icsk, list, icsk_listen_portaddr_node)
+
+static inline struct inet_listen_hashbucket *
+inet_lhash2_bucket(struct inet_hashinfo *h, u32 hash)
+{
+	return &h->lhash2[hash & h->lhash2_mask];
+}
+
 static inline struct inet_ehash_bucket *inet_ehash_bucket(
 	struct inet_hashinfo *hashinfo,
 	unsigned int hash)
@@ -209,6 +220,10 @@ int __inet_inherit_port(const struct sock *sk, struct sock *child);
 void inet_put_port(struct sock *sk);
 
 void inet_hashinfo_init(struct inet_hashinfo *h);
+void inet_hashinfo2_init(struct inet_hashinfo *h, const char *name,
+			 unsigned long numentries, int scale,
+			 unsigned long low_limit,
+			 unsigned long high_limit);
 
 bool inet_ehash_insert(struct sock *sk, struct sock *osk);
 bool inet_ehash_nolisten(struct sock *sk, struct sock *osk);
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 80cfd3fa21ca..f6f58108b4c5 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/wait.h>
 #include <linux/vmalloc.h>
+#include <linux/bootmem.h>
 
 #include <net/addrconf.h>
 #include <net/inet_connection_sock.h>
@@ -168,6 +169,60 @@ int __inet_inherit_port(const struct sock *sk, struct sock *child)
 }
 EXPORT_SYMBOL_GPL(__inet_inherit_port);
 
+static struct inet_listen_hashbucket *
+inet_lhash2_bucket_sk(struct inet_hashinfo *h, struct sock *sk)
+{
+	u32 hash;
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (sk->sk_family == AF_INET6)
+		hash = ipv6_portaddr_hash(sock_net(sk),
+					  &sk->sk_v6_rcv_saddr,
+					  inet_sk(sk)->inet_num);
+	else
+#endif
+		hash = ipv4_portaddr_hash(sock_net(sk),
+					  inet_sk(sk)->inet_rcv_saddr,
+					  inet_sk(sk)->inet_num);
+	return inet_lhash2_bucket(h, hash);
+}
+
+static void inet_hash2(struct inet_hashinfo *h, struct sock *sk)
+{
+	struct inet_listen_hashbucket *ilb2;
+
+	if (!h->lhash2)
+		return;
+
+	ilb2 = inet_lhash2_bucket_sk(h, sk);
+
+	spin_lock(&ilb2->lock);
+	if (sk->sk_reuseport && sk->sk_family == AF_INET6)
+		hlist_add_tail_rcu(&inet_csk(sk)->icsk_listen_portaddr_node,
+				   &ilb2->head);
+	else
+		hlist_add_head_rcu(&inet_csk(sk)->icsk_listen_portaddr_node,
+				   &ilb2->head);
+	ilb2->count++;
+	spin_unlock(&ilb2->lock);
+}
+
+static void inet_unhash2(struct inet_hashinfo *h, struct sock *sk)
+{
+	struct inet_listen_hashbucket *ilb2;
+
+	if (!h->lhash2 ||
+	    WARN_ON_ONCE(hlist_unhashed(&inet_csk(sk)->icsk_listen_portaddr_node)))
+		return;
+
+	ilb2 = inet_lhash2_bucket_sk(h, sk);
+
+	spin_lock(&ilb2->lock);
+	hlist_del_init_rcu(&inet_csk(sk)->icsk_listen_portaddr_node);
+	ilb2->count--;
+	spin_unlock(&ilb2->lock);
+}
+
 static inline int compute_score(struct sock *sk, struct net *net,
 				const unsigned short hnum, const __be32 daddr,
 				const int dif, const int sdif, bool exact_dif)
@@ -207,6 +262,40 @@ static inline int compute_score(struct sock *sk, struct net *net,
  */
 
 /* called with rcu_read_lock() : No refcount taken on the socket */
+static struct sock *inet_lhash2_lookup(struct net *net,
+				struct inet_listen_hashbucket *ilb2,
+				struct sk_buff *skb, int doff,
+				const __be32 saddr, __be16 sport,
+				const __be32 daddr, const unsigned short hnum,
+				const int dif, const int sdif)
+{
+	bool exact_dif = inet_exact_dif_match(net, skb);
+	struct inet_connection_sock *icsk;
+	struct sock *sk, *result = NULL;
+	int score, hiscore = 0;
+	u32 phash = 0;
+
+	inet_lhash2_for_each_icsk_rcu(icsk, &ilb2->head) {
+		sk = (struct sock *)icsk;
+		score = compute_score(sk, net, hnum, daddr,
+				      dif, sdif, exact_dif);
+		if (score > hiscore) {
+			if (sk->sk_reuseport) {
+				phash = inet_ehashfn(net, daddr, hnum,
+						     saddr, sport);
+				result = reuseport_select_sock(sk, phash,
+							       skb, doff);
+				if (result)
+					return result;
+			}
+			result = sk;
+			hiscore = score;
+		}
+	}
+
+	return result;
+}
+
 struct sock *__inet_lookup_listener(struct net *net,
 				    struct inet_hashinfo *hashinfo,
 				    struct sk_buff *skb, int doff,
@@ -217,10 +306,42 @@ struct sock *__inet_lookup_listener(struct net *net,
 	unsigned int hash = inet_lhashfn(net, hnum);
 	struct inet_listen_hashbucket *ilb = &hashinfo->listening_hash[hash];
 	bool exact_dif = inet_exact_dif_match(net, skb);
+	struct inet_listen_hashbucket *ilb2;
 	struct sock *sk, *result = NULL;
 	int score, hiscore = 0;
+	unsigned int hash2;
 	u32 phash = 0;
 
+	if (ilb->count <= 10 || !hashinfo->lhash2)
+		goto port_lookup;
+
+	/* Too many sk in the ilb bucket (which is hashed by port alone).
+	 * Try lhash2 (which is hashed by port and addr) instead.
+	 */
+
+	hash2 = ipv4_portaddr_hash(net, daddr, hnum);
+	ilb2 = inet_lhash2_bucket(hashinfo, hash2);
+	if (ilb2->count > ilb->count)
+		goto port_lookup;
+
+	result = inet_lhash2_lookup(net, ilb2, skb, doff,
+				    saddr, sport, daddr, hnum,
+				    dif, sdif);
+	if (result)
+		return result;
+
+	/* Lookup lhash2 with INADDR_ANY */
+
+	hash2 = ipv4_portaddr_hash(net, htonl(INADDR_ANY), hnum);
+	ilb2 = inet_lhash2_bucket(hashinfo, hash2);
+	if (ilb2->count > ilb->count)
+		goto port_lookup;
+
+	return inet_lhash2_lookup(net, ilb2, skb, doff,
+				  saddr, sport, daddr, hnum,
+				  dif, sdif);
+
+port_lookup:
 	sk_for_each_rcu(sk, &ilb->head) {
 		score = compute_score(sk, net, hnum, daddr,
 				      dif, sdif, exact_dif);
@@ -476,6 +597,7 @@ int __inet_hash(struct sock *sk, struct sock *osk)
 		hlist_add_tail_rcu(&sk->sk_node, &ilb->head);
 	else
 		hlist_add_head_rcu(&sk->sk_node, &ilb->head);
+	inet_hash2(hashinfo, sk);
 	ilb->count++;
 	sock_set_flag(sk, SOCK_RCU_FREE);
 	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
@@ -506,7 +628,6 @@ void inet_unhash(struct sock *sk)
 	struct inet_listen_hashbucket *ilb;
 	spinlock_t *lock;
 	bool listener = false;
-	int done;
 
 	if (sk_unhashed(sk))
 		return;
@@ -519,17 +640,20 @@ void inet_unhash(struct sock *sk)
 		lock = inet_ehash_lockp(hashinfo, sk->sk_hash);
 	}
 	spin_lock_bh(lock);
+	if (sk_unhashed(sk))
+		goto unlock;
+
 	if (rcu_access_pointer(sk->sk_reuseport_cb))
 		reuseport_detach_sock(sk);
-	if (listener)
-		done = __sk_del_node_init(sk);
-	else
-		done = __sk_nulls_del_node_init_rcu(sk);
-	if (done) {
-		if (listener)
-			ilb->count--;
-		sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
+	if (listener) {
+		inet_unhash2(hashinfo, sk);
+		 __sk_del_node_init(sk);
+		 ilb->count--;
+	} else {
+		__sk_nulls_del_node_init_rcu(sk);
 	}
+	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
+unlock:
 	spin_unlock_bh(lock);
 }
 EXPORT_SYMBOL_GPL(inet_unhash);
@@ -666,9 +790,35 @@ void inet_hashinfo_init(struct inet_hashinfo *h)
 		INIT_HLIST_HEAD(&h->listening_hash[i].head);
 		h->listening_hash[i].count = 0;
 	}
+
+	h->lhash2 = NULL;
 }
 EXPORT_SYMBOL_GPL(inet_hashinfo_init);
 
+void __init inet_hashinfo2_init(struct inet_hashinfo *h, const char *name,
+				unsigned long numentries, int scale,
+				unsigned long low_limit,
+				unsigned long high_limit)
+{
+	unsigned int i;
+
+	h->lhash2 = alloc_large_system_hash(name,
+					    sizeof(*h->lhash2),
+					    numentries,
+					    scale,
+					    0,
+					    NULL,
+					    &h->lhash2_mask,
+					    low_limit,
+					    high_limit);
+
+	for (i = 0; i <= h->lhash2_mask; i++) {
+		spin_lock_init(&h->lhash2[i].lock);
+		INIT_HLIST_HEAD(&h->lhash2[i].head);
+		h->lhash2[i].count = 0;
+	}
+}
+
 int inet_ehash_locks_alloc(struct inet_hashinfo *hashinfo)
 {
 	unsigned int locksz = sizeof(spinlock_t);
diff --git a/net/ipv6/inet6_hashtables.c b/net/ipv6/inet6_hashtables.c
index 0d1451381f5c..2febe26de6a1 100644
--- a/net/ipv6/inet6_hashtables.c
+++ b/net/ipv6/inet6_hashtables.c
@@ -125,6 +125,40 @@ static inline int compute_score(struct sock *sk, struct net *net,
 }
 
 /* called with rcu_read_lock() */
+static struct sock *inet6_lhash2_lookup(struct net *net,
+		struct inet_listen_hashbucket *ilb2,
+		struct sk_buff *skb, int doff,
+		const struct in6_addr *saddr,
+		const __be16 sport, const struct in6_addr *daddr,
+		const unsigned short hnum, const int dif, const int sdif)
+{
+	bool exact_dif = inet6_exact_dif_match(net, skb);
+	struct inet_connection_sock *icsk;
+	struct sock *sk, *result = NULL;
+	int score, hiscore = 0;
+	u32 phash = 0;
+
+	inet_lhash2_for_each_icsk_rcu(icsk, &ilb2->head) {
+		sk = (struct sock *)icsk;
+		score = compute_score(sk, net, hnum, daddr, dif, sdif,
+				      exact_dif);
+		if (score > hiscore) {
+			if (sk->sk_reuseport) {
+				phash = inet6_ehashfn(net, daddr, hnum,
+						      saddr, sport);
+				result = reuseport_select_sock(sk, phash,
+							       skb, doff);
+				if (result)
+					return result;
+			}
+			result = sk;
+			hiscore = score;
+		}
+	}
+
+	return result;
+}
+
 struct sock *inet6_lookup_listener(struct net *net,
 		struct inet_hashinfo *hashinfo,
 		struct sk_buff *skb, int doff,
@@ -135,10 +169,42 @@ struct sock *inet6_lookup_listener(struct net *net,
 	unsigned int hash = inet_lhashfn(net, hnum);
 	struct inet_listen_hashbucket *ilb = &hashinfo->listening_hash[hash];
 	bool exact_dif = inet6_exact_dif_match(net, skb);
+	struct inet_listen_hashbucket *ilb2;
 	struct sock *sk, *result = NULL;
 	int score, hiscore = 0;
+	unsigned int hash2;
 	u32 phash = 0;
 
+	if (ilb->count <= 10 || !hashinfo->lhash2)
+		goto port_lookup;
+
+	/* Too many sk in the ilb bucket (which is hashed by port alone).
+	 * Try lhash2 (which is hashed by port and addr) instead.
+	 */
+
+	hash2 = ipv6_portaddr_hash(net, daddr, hnum);
+	ilb2 = inet_lhash2_bucket(hashinfo, hash2);
+	if (ilb2->count > ilb->count)
+		goto port_lookup;
+
+	result = inet6_lhash2_lookup(net, ilb2, skb, doff,
+				     saddr, sport, daddr, hnum,
+				     dif, sdif);
+	if (result)
+		return result;
+
+	/* Lookup lhash2 with in6addr_any */
+
+	hash2 = ipv6_portaddr_hash(net, &in6addr_any, hnum);
+	ilb2 = inet_lhash2_bucket(hashinfo, hash2);
+	if (ilb2->count > ilb->count)
+		goto port_lookup;
+
+	return inet6_lhash2_lookup(net, ilb2, skb, doff,
+				   saddr, sport, daddr, hnum,
+				   dif, sdif);
+
+port_lookup:
 	sk_for_each(sk, &ilb->head) {
 		score = compute_score(sk, net, hnum, daddr, dif, sdif, exact_dif);
 		if (score > hiscore) {
-- 
2.9.5

^ permalink raw reply related

* [PATCH net-next 2/4] udp: Move udp[46]_portaddr_hash() to net/ip[v6].h
From: Martin KaFai Lau @ 2017-11-30 23:23 UTC (permalink / raw)
  To: netdev; +Cc: David S . Miller, Eric Dumazet, Kernel Team
In-Reply-To: <20171130232327.561108-1-kafai@fb.com>

This patch moves the udp[46]_portaddr_hash()
to net/ip[v6].h.  The function name is renamed to
ipv[46]_portaddr_hash().

It will be used by a later patch which adds a second listener
hashtable hashed by the address and port.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 include/net/ip.h   |  9 +++++++++
 include/net/ipv6.h | 17 +++++++++++++++++
 net/ipv4/udp.c     | 22 ++++++++--------------
 net/ipv6/udp.c     | 32 ++++++++------------------------
 4 files changed, 42 insertions(+), 38 deletions(-)

diff --git a/include/net/ip.h b/include/net/ip.h
index 9896f46cbbf1..fc9bf1b1fe2c 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -26,12 +26,14 @@
 #include <linux/ip.h>
 #include <linux/in.h>
 #include <linux/skbuff.h>
+#include <linux/jhash.h>
 
 #include <net/inet_sock.h>
 #include <net/route.h>
 #include <net/snmp.h>
 #include <net/flow.h>
 #include <net/flow_dissector.h>
+#include <net/netns/hash.h>
 
 #define IPV4_MAX_PMTU		65535U		/* RFC 2675, Section 5.1 */
 
@@ -521,6 +523,13 @@ static inline unsigned int ipv4_addr_hash(__be32 ip)
 	return (__force unsigned int) ip;
 }
 
+static inline u32 ipv4_portaddr_hash(const struct net *net,
+				     __be32 saddr,
+				     unsigned int port)
+{
+	return jhash_1word((__force u32)saddr, net_hash_mix(net)) ^ port;
+}
+
 bool ip_call_ra_chain(struct sk_buff *skb);
 
 /*
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index f73797e2fa60..25be4715578c 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -22,6 +22,7 @@
 #include <net/flow.h>
 #include <net/flow_dissector.h>
 #include <net/snmp.h>
+#include <net/netns/hash.h>
 
 #define SIN6_LEN_RFC2133	24
 
@@ -673,6 +674,22 @@ static inline bool ipv6_addr_v4mapped(const struct in6_addr *a)
 					cpu_to_be32(0x0000ffff))) == 0UL;
 }
 
+static inline u32 ipv6_portaddr_hash(const struct net *net,
+				     const struct in6_addr *addr6,
+				     unsigned int port)
+{
+	unsigned int hash, mix = net_hash_mix(net);
+
+	if (ipv6_addr_any(addr6))
+		hash = jhash_1word(0, mix);
+	else if (ipv6_addr_v4mapped(addr6))
+		hash = jhash_1word((__force u32)addr6->s6_addr32[3], mix);
+	else
+		hash = jhash2((__force u32 *)addr6->s6_addr32, 4, mix);
+
+	return hash ^ port;
+}
+
 /*
  * Check for a RFC 4843 ORCHID address
  * (Overlay Routable Cryptographic Hash Identifiers)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 36f857c87fe2..e9c0d1e1772e 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -357,18 +357,12 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum,
 }
 EXPORT_SYMBOL(udp_lib_get_port);
 
-static u32 udp4_portaddr_hash(const struct net *net, __be32 saddr,
-			      unsigned int port)
-{
-	return jhash_1word((__force u32)saddr, net_hash_mix(net)) ^ port;
-}
-
 int udp_v4_get_port(struct sock *sk, unsigned short snum)
 {
 	unsigned int hash2_nulladdr =
-		udp4_portaddr_hash(sock_net(sk), htonl(INADDR_ANY), snum);
+		ipv4_portaddr_hash(sock_net(sk), htonl(INADDR_ANY), snum);
 	unsigned int hash2_partial =
-		udp4_portaddr_hash(sock_net(sk), inet_sk(sk)->inet_rcv_saddr, 0);
+		ipv4_portaddr_hash(sock_net(sk), inet_sk(sk)->inet_rcv_saddr, 0);
 
 	/* precompute partial secondary hash */
 	udp_sk(sk)->udp_portaddr_hash = hash2_partial;
@@ -485,7 +479,7 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
 	u32 hash = 0;
 
 	if (hslot->count > 10) {
-		hash2 = udp4_portaddr_hash(net, daddr, hnum);
+		hash2 = ipv4_portaddr_hash(net, daddr, hnum);
 		slot2 = hash2 & udptable->mask;
 		hslot2 = &udptable->hash2[slot2];
 		if (hslot->count < hslot2->count)
@@ -496,7 +490,7 @@ struct sock *__udp4_lib_lookup(struct net *net, __be32 saddr,
 					  exact_dif, hslot2, skb);
 		if (!result) {
 			unsigned int old_slot2 = slot2;
-			hash2 = udp4_portaddr_hash(net, htonl(INADDR_ANY), hnum);
+			hash2 = ipv4_portaddr_hash(net, htonl(INADDR_ANY), hnum);
 			slot2 = hash2 & udptable->mask;
 			/* avoid searching the same slot again. */
 			if (unlikely(slot2 == old_slot2))
@@ -1761,7 +1755,7 @@ EXPORT_SYMBOL(udp_lib_rehash);
 
 static void udp_v4_rehash(struct sock *sk)
 {
-	u16 new_hash = udp4_portaddr_hash(sock_net(sk),
+	u16 new_hash = ipv4_portaddr_hash(sock_net(sk),
 					  inet_sk(sk)->inet_rcv_saddr,
 					  inet_sk(sk)->inet_num);
 	udp_lib_rehash(sk, new_hash);
@@ -1952,9 +1946,9 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	struct sk_buff *nskb;
 
 	if (use_hash2) {
-		hash2_any = udp4_portaddr_hash(net, htonl(INADDR_ANY), hnum) &
+		hash2_any = ipv4_portaddr_hash(net, htonl(INADDR_ANY), hnum) &
 			    udptable->mask;
-		hash2 = udp4_portaddr_hash(net, daddr, hnum) & udptable->mask;
+		hash2 = ipv4_portaddr_hash(net, daddr, hnum) & udptable->mask;
 start_lookup:
 		hslot = &udptable->hash2[hash2];
 		offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
@@ -2186,7 +2180,7 @@ static struct sock *__udp4_lib_demux_lookup(struct net *net,
 					    int dif, int sdif)
 {
 	unsigned short hnum = ntohs(loc_port);
-	unsigned int hash2 = udp4_portaddr_hash(net, loc_addr, hnum);
+	unsigned int hash2 = ipv4_portaddr_hash(net, loc_addr, hnum);
 	unsigned int slot2 = hash2 & udp_table.mask;
 	struct udp_hslot *hslot2 = &udp_table.hash2[slot2];
 	INET_ADDR_COOKIE(acookie, rmt_addr, loc_addr);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index c9f91c28b81d..eecf9f0faf29 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -89,28 +89,12 @@ static u32 udp6_ehashfn(const struct net *net,
 			       udp_ipv6_hash_secret + net_hash_mix(net));
 }
 
-static u32 udp6_portaddr_hash(const struct net *net,
-			      const struct in6_addr *addr6,
-			      unsigned int port)
-{
-	unsigned int hash, mix = net_hash_mix(net);
-
-	if (ipv6_addr_any(addr6))
-		hash = jhash_1word(0, mix);
-	else if (ipv6_addr_v4mapped(addr6))
-		hash = jhash_1word((__force u32)addr6->s6_addr32[3], mix);
-	else
-		hash = jhash2((__force u32 *)addr6->s6_addr32, 4, mix);
-
-	return hash ^ port;
-}
-
 int udp_v6_get_port(struct sock *sk, unsigned short snum)
 {
 	unsigned int hash2_nulladdr =
-		udp6_portaddr_hash(sock_net(sk), &in6addr_any, snum);
+		ipv6_portaddr_hash(sock_net(sk), &in6addr_any, snum);
 	unsigned int hash2_partial =
-		udp6_portaddr_hash(sock_net(sk), &sk->sk_v6_rcv_saddr, 0);
+		ipv6_portaddr_hash(sock_net(sk), &sk->sk_v6_rcv_saddr, 0);
 
 	/* precompute partial secondary hash */
 	udp_sk(sk)->udp_portaddr_hash = hash2_partial;
@@ -119,7 +103,7 @@ int udp_v6_get_port(struct sock *sk, unsigned short snum)
 
 static void udp_v6_rehash(struct sock *sk)
 {
-	u16 new_hash = udp6_portaddr_hash(sock_net(sk),
+	u16 new_hash = ipv6_portaddr_hash(sock_net(sk),
 					  &sk->sk_v6_rcv_saddr,
 					  inet_sk(sk)->inet_num);
 
@@ -225,7 +209,7 @@ struct sock *__udp6_lib_lookup(struct net *net,
 	u32 hash = 0;
 
 	if (hslot->count > 10) {
-		hash2 = udp6_portaddr_hash(net, daddr, hnum);
+		hash2 = ipv6_portaddr_hash(net, daddr, hnum);
 		slot2 = hash2 & udptable->mask;
 		hslot2 = &udptable->hash2[slot2];
 		if (hslot->count < hslot2->count)
@@ -236,7 +220,7 @@ struct sock *__udp6_lib_lookup(struct net *net,
 					  hslot2, skb);
 		if (!result) {
 			unsigned int old_slot2 = slot2;
-			hash2 = udp6_portaddr_hash(net, &in6addr_any, hnum);
+			hash2 = ipv6_portaddr_hash(net, &in6addr_any, hnum);
 			slot2 = hash2 & udptable->mask;
 			/* avoid searching the same slot again. */
 			if (unlikely(slot2 == old_slot2))
@@ -705,9 +689,9 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	struct sk_buff *nskb;
 
 	if (use_hash2) {
-		hash2_any = udp6_portaddr_hash(net, &in6addr_any, hnum) &
+		hash2_any = ipv6_portaddr_hash(net, &in6addr_any, hnum) &
 			    udptable->mask;
-		hash2 = udp6_portaddr_hash(net, daddr, hnum) & udptable->mask;
+		hash2 = ipv6_portaddr_hash(net, daddr, hnum) & udptable->mask;
 start_lookup:
 		hslot = &udptable->hash2[hash2];
 		offset = offsetof(typeof(*sk), __sk_common.skc_portaddr_node);
@@ -895,7 +879,7 @@ static struct sock *__udp6_lib_demux_lookup(struct net *net,
 			int dif, int sdif)
 {
 	unsigned short hnum = ntohs(loc_port);
-	unsigned int hash2 = udp6_portaddr_hash(net, loc_addr, hnum);
+	unsigned int hash2 = ipv6_portaddr_hash(net, loc_addr, hnum);
 	unsigned int slot2 = hash2 & udp_table.mask;
 	struct udp_hslot *hslot2 = &udp_table.hash2[slot2];
 	const __portpair ports = INET_COMBINED_PORTS(rmt_port, hnum);
-- 
2.9.5

^ permalink raw reply related

* Re: [PATCH net-next 3/5] net: phy: phylink: Demote error message to debug
From: Russell King - ARM Linux @ 2017-11-30 23:26 UTC (permalink / raw)
  To: Florian Fainelli; +Cc: netdev, davem, andrew, vivien.didelot
In-Reply-To: <20171130195744.17743-4-f.fainelli@gmail.com>

On Thu, Nov 30, 2017 at 11:57:42AM -0800, Florian Fainelli wrote:
> Some subsystems like DSA may be trying to connect to a PHY through OF first,
> and then attempt a connect using a local MDIO bus, demote the error message:
> "unable to find PHY node" into a debug print.

Maybe it would be better to push this out to the drivers to print
if they wish?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

^ permalink raw reply

* Re: [PATCH net-next 3/5] net: phy: phylink: Demote error message to debug
From: Florian Fainelli @ 2017-11-30 23:36 UTC (permalink / raw)
  To: Russell King - ARM Linux; +Cc: netdev, davem, andrew, vivien.didelot
In-Reply-To: <20171130232624.GH10595@n2100.armlinux.org.uk>

On 11/30/2017 03:26 PM, Russell King - ARM Linux wrote:
> On Thu, Nov 30, 2017 at 11:57:42AM -0800, Florian Fainelli wrote:
>> Some subsystems like DSA may be trying to connect to a PHY through OF first,
>> and then attempt a connect using a local MDIO bus, demote the error message:
>> "unable to find PHY node" into a debug print.
> 
> Maybe it would be better to push this out to the drivers to print
> if they wish?
> 

Sure, I will just drop that message then.
-- 
Florian

^ permalink raw reply

* [PATCH v3 0/6] enable creating [k,u]probe with perf_event_open
From: Song Liu @ 2017-11-30 23:50 UTC (permalink / raw)
  To: peterz, rostedt, mingo, davem, netdev, linux-kernel, daniel
  Cc: kernel-team, Song Liu

Changes PATCH v2 to PATCH v3:
  Remove fixed type PERF_TYPE_KPROBE and PERF_TYPE_UPROBE, use dynamic
  type instead.
  Update userspace (samples/bpf, bcc) to look up type from sysfs.
  Change License info in test_many_kprobe_user.c as Philippe Ombredanne
  suggested.

Changes PATCH v1 to PATCH v2:
  Split PERF_TYPE_PROBE into PERF_TYPE_KPROBE and PERF_TYPE_UPROBE.
  Split perf_probe into perf_kprobe and perf_uprobe.
  Remove struct probe_desc, use config1 and config2 instead.

Changes RFC v2 to PATCH v1:
  Check type PERF_TYPE_PROBE in perf_event_set_filter().
  Rebase on to tip perf/core.

Changes RFC v1 to RFC v2:
  Fix build issue reported by kbuild test bot by adding ifdef of
  CONFIG_KPROBE_EVENTS, and CONFIG_UPROBE_EVENTS.

RFC v1 cover letter:

This is to follow up the discussion over "new kprobe api" at Linux
Plumbers 2017:

https://www.linuxplumbersconf.org/2017/ocw/proposals/4808

With current kernel, user space tools can only create/destroy [k,u]probes
with a text-based API (kprobe_events and uprobe_events in tracefs). This
approach relies on user space to clean up the [k,u]probe after using them.
However, this is not easy for user space to clean up properly.

To solve this problem, we introduce a file descriptor based API.
Specifically, we extended perf_event_open to create [k,u]probe, and attach
this [k,u]probe to the file descriptor created by perf_event_open. These
[k,u]probe are associated with this file descriptor, so they are not
available in tracefs.

We reuse large portion of existing trace_kprobe and trace_uprobe code.
Currently, the file descriptor API does not support arguments as the
text-based API does. This should not be a problem, as user of the file
decriptor based API read data through other methods (bpf, etc.).

I also include a patch to to bcc, and a patch to man-page perf_even_open.
Please see the list below. A fork of bcc with this patch is also available
on github:

  https://github.com/liu-song-6/bcc/tree/perf_event_open

Thanks,
Song

man-pages patch:
  perf_event_open.2: add type kprobe and uprobe

bcc patch:
  bcc: Try use new API to create [k,u]probe with perf_event_open

kernel patches:

Song Liu (6):
  perf: prepare perf_event.h for new types perf_kprobe and perf_uprobe
  perf: copy new perf_event.h to tools/include/uapi
  perf: implement pmu perf_kprobe
  perf: implement pmu perf_uprobe
  bpf: add option for bpf_load.c to use perf_kprobe
  bpf: add new test test_many_kprobe

 include/linux/trace_events.h          |   4 +
 include/uapi/linux/perf_event.h       |   6 ++
 kernel/events/core.c                  |  81 ++++++++++++++-
 kernel/trace/trace_event_perf.c       | 111 ++++++++++++++++++++
 kernel/trace/trace_kprobe.c           |  91 +++++++++++++++--
 kernel/trace/trace_probe.h            |  11 ++
 kernel/trace/trace_uprobe.c           |  86 ++++++++++++++--
 samples/bpf/Makefile                  |   3 +
 samples/bpf/bpf_load.c                |  63 ++++++++++--
 samples/bpf/bpf_load.h                |  14 +++
 samples/bpf/test_many_kprobe_user.c   | 186 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/perf_event.h |   6 ++
 12 files changed, 633 insertions(+), 29 deletions(-)
 create mode 100644 samples/bpf/test_many_kprobe_user.c

--
2.9.5

^ permalink raw reply

* [PATCH v3] perf_event_open.2: add type kprobe and uprobe
From: Song Liu @ 2017-11-30 23:50 UTC (permalink / raw)
  To: peterz, rostedt, mingo, davem, netdev, linux-kernel, daniel
  Cc: kernel-team, Song Liu
In-Reply-To: <20171130235023.1414663-1-songliubraving@fb.com>

Two new types kprobe and uprobe are being added to perf_event_open,
which allow creating kprobe or uprobe with perf_event_open. This
patch adds information about these types.

Signed-off-by: Song Liu <songliubraving@fb.com>
---
 man2/perf_event_open.2 | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/man2/perf_event_open.2 b/man2/perf_event_open.2
index c91da3f..e80050f 100644
--- a/man2/perf_event_open.2
+++ b/man2/perf_event_open.2
@@ -256,11 +256,15 @@ struct perf_event_attr {
 
     union {
         __u64 bp_addr;          /* breakpoint address */
+        __u64 kprobe_func;      /* for perf_kprobe */
+        __u64 uprobe_path;      /* for perf_uprobe */
         __u64 config1;          /* extension of config */
     };
 
     union {
         __u64 bp_len;           /* breakpoint length */
+        __u64 kprobe_addr;      /* with kprobe_func == NULL */
+        __u64 probe_offset;     /* for perf_[k,u]probe */
         __u64 config2;          /* extension of config1 */
     };
     __u64 branch_sample_type;   /* enum perf_branch_sample_type */
@@ -336,6 +340,13 @@ field.
 For instance,
 .I /sys/bus/event_source/devices/cpu/type
 contains the value for the core CPU PMU, which is usually 4.
+.TP
+.BR kprobe " and " uprobe " (since Linux 4.TBD)"
+These two dynamic PMU creates kprobe or uprobe with perf_event_open and
+attaches it to the file descriptor.
+See fields
+.IR kprobe_func ", " uprobe_path ", " kprobe_addr ", and " probe_offset
+for more details.
 .RE
 .TP
 .I "size"
@@ -627,6 +638,43 @@ then leave
 .I config
 set to zero.
 Its parameters are set in other places.
+.PP
+If
+.I type
+is
+.BR kprobe
+or
+.BR uprobe ,
+.I config
+of 0 means kprobe/uprobe, while
+.I config
+of 1 means kretprobe/uretprobe. See fields
+.IR kprobe_func ", " uprobe_path ", " kprobe_addr ", and " probe_offset
+for more details.
+.RE
+.TP
+.IR kprobe_func ", " uprobe_path ", " kprobe_addr ", and " probe_offset
+.EE
+These fields describes the kprobe/uprobe for dynamic PMU
+.BR kprobe
+and
+.BR uprobe .
+For
+.BR kprobe ": "
+use
+.I kprobe_func
+and
+.IR probe_offset ,
+or use
+.I kprobe_addr
+and leave
+.I kprobe_func
+as NULL. For
+.BR uprobe ": "
+use
+.I uprobe_path
+and
+.IR probe_offset .
 .RE
 .TP
 .IR sample_period ", " sample_freq
-- 
2.9.5

^ permalink raw reply related

* [PATCH v3 2/6] perf: copy new perf_event.h to tools/include/uapi
From: Song Liu @ 2017-11-30 23:50 UTC (permalink / raw)
  To: peterz, rostedt, mingo, davem, netdev, linux-kernel, daniel
  Cc: kernel-team, Song Liu
In-Reply-To: <20171130235023.1414663-1-songliubraving@fb.com>

perf_event.h is updated in previous patch, this patch applies same
changes to the tools/ version. This is part is put in a separate
patch in case the two files are back ported separately.

Signed-off-by: Song Liu <songliubraving@fb.com>
Reviewed-by: Yonghong Song <yhs@fb.com>
Reviewed-by: Josef Bacik <jbacik@fb.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/include/uapi/linux/perf_event.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
index b9a4953..2df7ddf 100644
--- a/tools/include/uapi/linux/perf_event.h
+++ b/tools/include/uapi/linux/perf_event.h
@@ -299,6 +299,8 @@ enum perf_event_read_format {
 #define PERF_ATTR_SIZE_VER4	104	/* add: sample_regs_intr */
 #define PERF_ATTR_SIZE_VER5	112	/* add: aux_watermark */
 
+#define MAX_PROBE_FUNC_NAME_LEN 64
+
 /*
  * Hardware event_id to monitor via a performance monitoring event:
  *
@@ -380,10 +382,14 @@ struct perf_event_attr {
 	__u32			bp_type;
 	union {
 		__u64		bp_addr;
+		__u64		kprobe_func; /* for perf_kprobe */
+		__u64		uprobe_path; /* for perf_uprobe */
 		__u64		config1; /* extension of config */
 	};
 	union {
 		__u64		bp_len;
+		__u64		kprobe_addr; /* when kprobe_func == NULL */
+		__u64		probe_offset; /* for perf_[k,u]probe */
 		__u64		config2; /* extension of config1 */
 	};
 	__u64	branch_sample_type; /* enum perf_branch_sample_type */
-- 
2.9.5

^ permalink raw reply related

* [PATCH v3 5/6] bpf: add option for bpf_load.c to use perf_kprobe
From: Song Liu @ 2017-11-30 23:50 UTC (permalink / raw)
  To: peterz, rostedt, mingo, davem, netdev, linux-kernel, daniel
  Cc: kernel-team, Song Liu
In-Reply-To: <20171130235023.1414663-1-songliubraving@fb.com>

Function load_and_attach() is updated to be able to create kprobes
with either old text based API, or the new perf_event_open API.

A global flag use_perf_kprobe is added to select between the two
APIs.

Signed-off-by: Song Liu <songliubraving@fb.com>
Reviewed-by: Josef Bacik <jbacik@fb.com>
---
 samples/bpf/bpf_load.c | 58 +++++++++++++++++++++++++++++++++++++++++++-------
 samples/bpf/bpf_load.h | 10 +++++++++
 2 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index 2325d7a..da19e58 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -8,7 +8,6 @@
 #include <errno.h>
 #include <unistd.h>
 #include <string.h>
-#include <stdbool.h>
 #include <stdlib.h>
 #include <linux/bpf.h>
 #include <linux/filter.h>
@@ -29,6 +28,7 @@
 #include "perf-sys.h"
 
 #define DEBUGFS "/sys/kernel/debug/tracing/"
+#define KPROBE_TYPE_FILE "/sys/bus/event_source/devices/kprobe/type"
 
 static char license[128];
 static int kern_version;
@@ -42,6 +42,8 @@ int prog_array_fd = -1;
 
 struct bpf_map_data map_data[MAX_MAPS];
 int map_data_count = 0;
+bool use_perf_kprobe = true;
+int perf_kprobe_type = -1;
 
 static int populate_prog_array(const char *event, int prog_fd)
 {
@@ -55,6 +57,26 @@ static int populate_prog_array(const char *event, int prog_fd)
 	return 0;
 }
 
+int get_perf_kprobe_type_id(void)
+{
+	int tfd;
+	int err;
+	char buf[16];
+
+	tfd = open(KPROBE_TYPE_FILE, O_RDONLY);
+	if (tfd < 0)
+		return -1;
+
+	err = read(tfd, buf, sizeof(buf));
+	close(tfd);
+
+	if (err < 0 || err >= sizeof(buf))
+		return -1;
+	buf[err] = 0;
+	perf_kprobe_type = atoi(buf);
+	return perf_kprobe_type;
+}
+
 static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
 {
 	bool is_socket = strncmp(event, "socket", 6) == 0;
@@ -70,7 +92,7 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
 	size_t insns_cnt = size / sizeof(struct bpf_insn);
 	enum bpf_prog_type prog_type;
 	char buf[256];
-	int fd, efd, err, id;
+	int fd, efd, err, id = -1;
 	struct perf_event_attr attr = {};
 
 	attr.type = PERF_TYPE_TRACEPOINT;
@@ -128,7 +150,13 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
 		return populate_prog_array(event, fd);
 	}
 
-	if (is_kprobe || is_kretprobe) {
+	if (use_perf_kprobe && perf_kprobe_type == -1) {
+		get_perf_kprobe_type_id();
+		if (perf_kprobe_type == -1)
+			use_perf_kprobe = false;
+	}
+
+	if (!use_perf_kprobe && (is_kprobe || is_kretprobe)) {
 		if (is_kprobe)
 			event += 7;
 		else
@@ -169,27 +197,41 @@ static int load_and_attach(const char *event, struct bpf_insn *prog, int size)
 		strcat(buf, "/id");
 	}
 
+	if (use_perf_kprobe && (is_kprobe || is_kretprobe)) {
+		attr.type = perf_kprobe_type;
+		attr.kprobe_func = ptr_to_u64(
+			event + strlen(is_kprobe ? "kprobe/" : "kretprobe/"));
+		attr.probe_offset = 0;
+		attr.config  = !!is_kretprobe;
+	} else {
 		efd = open(buf, O_RDONLY, 0);
 		if (efd < 0) {
 			printf("failed to open event %s\n", event);
 			return -1;
 		}
-
 		err = read(efd, buf, sizeof(buf));
 		if (err < 0 || err >= sizeof(buf)) {
-		printf("read from '%s' failed '%s'\n", event, strerror(errno));
+			printf("read from '%s' failed '%s'\n", event,
+			       strerror(errno));
 			return -1;
 		}
-
 		close(efd);
-
 		buf[err] = 0;
 		id = atoi(buf);
 		attr.config = id;
+	}
 
 	efd = sys_perf_event_open(&attr, -1/*pid*/, 0/*cpu*/, -1/*group_fd*/, 0);
 	if (efd < 0) {
-		printf("event %d fd %d err %s\n", id, efd, strerror(errno));
+		if (use_perf_kprobe && (is_kprobe || is_kretprobe))
+			printf("k%sprobe %s fd %d err %s\n",
+			       is_kprobe ? "" : "ret",
+			       event + strlen(is_kprobe ? "kprobe/"
+					      : "kretprobe/"),
+			       efd, strerror(errno));
+		else
+			printf("event %d fd %d err %s\n", id, efd,
+			       strerror(errno));
 		return -1;
 	}
 	event_fd[prog_cnt - 1] = efd;
diff --git a/samples/bpf/bpf_load.h b/samples/bpf/bpf_load.h
index 7d57a42..95d6be5 100644
--- a/samples/bpf/bpf_load.h
+++ b/samples/bpf/bpf_load.h
@@ -2,6 +2,7 @@
 #ifndef __BPF_LOAD_H
 #define __BPF_LOAD_H
 
+#include <stdbool.h>
 #include "libbpf.h"
 
 #define MAX_MAPS 32
@@ -38,6 +39,10 @@ extern int map_fd[MAX_MAPS];
 extern struct bpf_map_data map_data[MAX_MAPS];
 extern int map_data_count;
 
+extern bool use_perf_kprobe;
+extern int perf_kprobe_type;
+extern int get_perf_kprobe_type_id(void);
+
 /* parses elf file compiled by llvm .c->.o
  * . parses 'maps' section and creates maps via BPF syscall
  * . parses 'license' section and passes it to syscall
@@ -59,6 +64,11 @@ struct ksym {
 	char *name;
 };
 
+static inline __u64 ptr_to_u64(const void *ptr)
+{
+	return (__u64) (unsigned long) ptr;
+}
+
 int load_kallsyms(void);
 struct ksym *ksym_search(long key);
 int set_link_xdp_fd(int ifindex, int fd, __u32 flags);
-- 
2.9.5

^ permalink raw reply related

* [PATCH v3 6/6] bpf: add new test test_many_kprobe
From: Song Liu @ 2017-11-30 23:50 UTC (permalink / raw)
  To: peterz, rostedt, mingo, davem, netdev, linux-kernel, daniel
  Cc: kernel-team, Song Liu
In-Reply-To: <20171130235023.1414663-1-songliubraving@fb.com>

The test compares old text based kprobe API with perf_kprobe.

Here is a sample output of this test:

Creating 1000 kprobes with text-based API takes 6.979683 seconds
Cleaning 1000 kprobes with text-based API takes 84.897687 seconds
Creating 1000 kprobes with perf_kprobe (function name) takes 5.077558 seconds
Cleaning 1000 kprobes with perf_kprobe (function name) takes 81.241354 seconds
Creating 1000 kprobes with perf_kprobe (function addr) takes 5.218255 seconds
Cleaning 1000 kprobes with perf_kprobe (function addr) takes 80.010731 seconds

Signed-off-by: Song Liu <songliubraving@fb.com>
Reviewed-by: Josef Bacik <jbacik@fb.com>
---
 samples/bpf/Makefile                |   3 +
 samples/bpf/bpf_load.c              |   5 +-
 samples/bpf/bpf_load.h              |   4 +
 samples/bpf/test_many_kprobe_user.c | 186 ++++++++++++++++++++++++++++++++++++
 4 files changed, 195 insertions(+), 3 deletions(-)
 create mode 100644 samples/bpf/test_many_kprobe_user.c

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 9b4a66e..ec92f35 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -42,6 +42,7 @@ hostprogs-y += xdp_redirect
 hostprogs-y += xdp_redirect_map
 hostprogs-y += xdp_monitor
 hostprogs-y += syscall_tp
+hostprogs-y += test_many_kprobe
 
 # Libbpf dependencies
 LIBBPF := ../../tools/lib/bpf/bpf.o
@@ -87,6 +88,7 @@ xdp_redirect-objs := bpf_load.o $(LIBBPF) xdp_redirect_user.o
 xdp_redirect_map-objs := bpf_load.o $(LIBBPF) xdp_redirect_map_user.o
 xdp_monitor-objs := bpf_load.o $(LIBBPF) xdp_monitor_user.o
 syscall_tp-objs := bpf_load.o $(LIBBPF) syscall_tp_user.o
+test_many_kprobe-objs := bpf_load.o $(LIBBPF) test_many_kprobe_user.o
 
 # Tell kbuild to always build the programs
 always := $(hostprogs-y)
@@ -172,6 +174,7 @@ HOSTLOADLIBES_xdp_redirect += -lelf
 HOSTLOADLIBES_xdp_redirect_map += -lelf
 HOSTLOADLIBES_xdp_monitor += -lelf
 HOSTLOADLIBES_syscall_tp += -lelf
+HOSTLOADLIBES_test_many_kprobe += -lelf
 
 # Allows pointing LLC/CLANG to a LLVM backend with bpf support, redefine on cmdline:
 #  make samples/bpf/ LLC=~/git/llvm/build/bin/llc CLANG=~/git/llvm/build/bin/clang
diff --git a/samples/bpf/bpf_load.c b/samples/bpf/bpf_load.c
index da19e58..6f38db0 100644
--- a/samples/bpf/bpf_load.c
+++ b/samples/bpf/bpf_load.c
@@ -663,9 +663,8 @@ void read_trace_pipe(void)
 	}
 }
 
-#define MAX_SYMS 300000
-static struct ksym syms[MAX_SYMS];
-static int sym_cnt;
+struct ksym syms[MAX_SYMS];
+int sym_cnt;
 
 static int ksym_cmp(const void *p1, const void *p2)
 {
diff --git a/samples/bpf/bpf_load.h b/samples/bpf/bpf_load.h
index 95d6be5..6c9d584 100644
--- a/samples/bpf/bpf_load.h
+++ b/samples/bpf/bpf_load.h
@@ -69,6 +69,10 @@ static inline __u64 ptr_to_u64(const void *ptr)
 	return (__u64) (unsigned long) ptr;
 }
 
+#define MAX_SYMS 300000
+extern struct ksym syms[MAX_SYMS];
+extern int sym_cnt;
+
 int load_kallsyms(void);
 struct ksym *ksym_search(long key);
 int set_link_xdp_fd(int ifindex, int fd, __u32 flags);
diff --git a/samples/bpf/test_many_kprobe_user.c b/samples/bpf/test_many_kprobe_user.c
new file mode 100644
index 0000000..6c111cf
--- /dev/null
+++ b/samples/bpf/test_many_kprobe_user.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017 Facebook
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <string.h>
+#include <libelf.h>
+#include <gelf.h>
+#include <linux/version.h>
+#include <errno.h>
+#include <stdbool.h>
+#include <time.h>
+#include "libbpf.h"
+#include "bpf_load.h"
+#include "perf-sys.h"
+
+#define MAX_KPROBES 1000
+
+#define DEBUGFS "/sys/kernel/debug/tracing/"
+
+int kprobes[MAX_KPROBES] = {0};
+int kprobe_count;
+int perf_event_fds[MAX_KPROBES];
+const char license[] = "GPL";
+
+static __u64 time_get_ns(void)
+{
+	struct timespec ts;
+
+	clock_gettime(CLOCK_MONOTONIC, &ts);
+	return ts.tv_sec * 1000000000ull + ts.tv_nsec;
+}
+
+static int kprobe_api(char *func, void *addr, bool use_new_api)
+{
+	int efd;
+	struct perf_event_attr attr = {};
+	char buf[256];
+	int err, id;
+
+	attr.sample_type = PERF_SAMPLE_RAW;
+	attr.sample_period = 1;
+	attr.wakeup_events = 1;
+
+	if (use_new_api) {
+		attr.type = perf_kprobe_type;
+		if (func) {
+			attr.kprobe_func = ptr_to_u64(func);
+			attr.probe_offset = 0;
+		} else {
+			attr.kprobe_func = 0;
+			attr.kprobe_addr = ptr_to_u64(addr);
+		}
+	} else {
+		attr.type = PERF_TYPE_TRACEPOINT;
+		snprintf(buf, sizeof(buf),
+			 "echo 'p:%s %s' >> /sys/kernel/debug/tracing/kprobe_events",
+			 func, func);
+		err = system(buf);
+		if (err < 0) {
+			printf("failed to create kprobe '%s' error '%s'\n",
+			       func, strerror(errno));
+			return -1;
+		}
+
+		strcpy(buf, DEBUGFS);
+		strcat(buf, "events/kprobes/");
+		strcat(buf, func);
+		strcat(buf, "/id");
+		efd = open(buf, O_RDONLY, 0);
+		if (efd < 0) {
+			printf("failed to open event %s\n", func);
+			return -1;
+		}
+
+		err = read(efd, buf, sizeof(buf));
+		if (err < 0 || err >= sizeof(buf)) {
+			printf("read from '%s' failed '%s'\n", func,
+			       strerror(errno));
+			return -1;
+		}
+
+		close(efd);
+		buf[err] = 0;
+		id = atoi(buf);
+		attr.config = id;
+	}
+
+	attr.size = sizeof(attr);
+	efd = sys_perf_event_open(&attr, -1/*pid*/, 0/*cpu*/,
+				  -1/*group_fd*/, 0);
+
+	return efd;
+}
+
+static int select_kprobes(void)
+{
+	int fd;
+	int i;
+
+	load_kallsyms();
+
+	kprobe_count = 0;
+	for (i = 0; i < sym_cnt; i++) {
+		if (strstr(syms[i].name, "."))
+			continue;
+		fd = kprobe_api(syms[i].name, NULL, false);
+		if (fd < 0)
+			continue;
+		close(fd);
+		kprobes[kprobe_count] = i;
+		if (++kprobe_count >= MAX_KPROBES)
+			break;
+	}
+
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	int i;
+	__u64 start_time;
+
+	select_kprobes();
+
+	/* clean all trace_kprobe */
+	i = system("echo \"\" > /sys/kernel/debug/tracing/kprobe_events");
+
+	/* test text based API */
+	start_time = time_get_ns();
+	for (i = 0; i < kprobe_count; i++)
+		perf_event_fds[i] = kprobe_api(syms[kprobes[i]].name,
+					       NULL, false);
+	printf("Creating %d kprobes with text-based API takes %f seconds\n",
+	       kprobe_count, (time_get_ns() - start_time) / 1000000000.0);
+
+	start_time = time_get_ns();
+	for (i = 0; i < kprobe_count; i++)
+		if (perf_event_fds[i] > 0)
+			close(perf_event_fds[i]);
+	i = system("echo \"\" > /sys/kernel/debug/tracing/kprobe_events");
+	printf("Cleaning %d kprobes with text-based API takes %f seconds\n",
+	       kprobe_count, (time_get_ns() - start_time) / 1000000000.0);
+
+	get_perf_kprobe_type_id();
+	if (perf_kprobe_type == -1) {
+		printf("The kernel does support perf_kprobe.\n"
+		       "Existing...\n");
+		return 0;
+	}
+
+	/* test perf_kprobe API, with function names */
+	start_time = time_get_ns();
+	for (i = 0; i < kprobe_count; i++)
+		perf_event_fds[i] = kprobe_api(syms[kprobes[i]].name,
+					       NULL, true);
+	printf("Creating %d kprobes with perf_kprobe (function name) takes %f seconds\n",
+	       kprobe_count, (time_get_ns() - start_time) / 1000000000.0);
+
+	start_time = time_get_ns();
+	for (i = 0; i < kprobe_count; i++)
+		if (perf_event_fds[i] > 0)
+			close(perf_event_fds[i]);
+	printf("Cleaning %d kprobes with perf_kprobe (function name) takes %f seconds\n",
+	       kprobe_count, (time_get_ns() - start_time) / 1000000000.0);
+
+	/* test perf_kprobe API, with function address */
+	start_time = time_get_ns();
+	for (i = 0; i < kprobe_count; i++)
+		perf_event_fds[i] = kprobe_api(
+			NULL, (void *)(syms[kprobes[i]].addr), true);
+	printf("Creating %d kprobes with perf_kprobe (function addr) takes %f seconds\n",
+	       kprobe_count, (time_get_ns() - start_time) / 1000000000.0);
+
+	start_time = time_get_ns();
+	for (i = 0; i < kprobe_count; i++)
+		if (perf_event_fds[i] > 0)
+			close(perf_event_fds[i]);
+	printf("Cleaning %d kprobes with perf_kprobe (function addr) takes %f seconds\n",
+	       kprobe_count, (time_get_ns() - start_time) / 1000000000.0);
+	return 0;
+}
-- 
2.9.5

^ permalink raw reply related

* [PATCH v3] bcc: Try use new API to create [k,u]probe with perf_event_open
From: Song Liu @ 2017-11-30 23:50 UTC (permalink / raw)
  To: peterz, rostedt, mingo, davem, netdev, linux-kernel, daniel
  Cc: kernel-team, Song Liu
In-Reply-To: <20171130235023.1414663-1-songliubraving@fb.com>

New kernel API allows creating [k,u]probe with perf_event_open.
This patch tries to use the new API. If the new API doesn't work,
we fall back to old API.

bpf_detach_probe() looks up the event being removed. If the event
is not found, we skip the clean up procedure.

Signed-off-by: Song Liu <songliubraving@fb.com>
---
 src/cc/libbpf.c | 245 ++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 176 insertions(+), 69 deletions(-)

diff --git a/src/cc/libbpf.c b/src/cc/libbpf.c
index ef6daf3..5be9a7b 100644
--- a/src/cc/libbpf.c
+++ b/src/cc/libbpf.c
@@ -526,38 +526,67 @@ int bpf_attach_socket(int sock, int prog) {
   return setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &prog, sizeof(prog));
 }
 
+/*
+ * new kernel API allows creating [k,u]probe with perf_event_open, which
+ * makes it easier to clean up the [k,u]probe. This function tries to
+ * create pfd with the new API.
+ */
+static int bpf_try_perf_event_open_with_probe(const char *name, uint64_t offs,
+    int pid, int cpu, int group_fd, int perf_probe_type, int is_return)
+{
+  struct perf_event_attr attr = {};
+
+  attr.sample_type = PERF_SAMPLE_RAW | PERF_SAMPLE_CALLCHAIN;
+  attr.sample_period = 1;
+  attr.wakeup_events = 1;
+  attr.config = is_return ? 1 : 0;
+  attr.probe_offset = offs;  /* for kprobe, if name is NULL, this the addr */
+  attr.size = sizeof(attr);
+  attr.type = perf_probe_type;
+  attr.kprobe_func = ptr_to_u64((void *)name);  /* also work for uprobe_path */
+  return syscall(__NR_perf_event_open, &attr, pid, cpu, group_fd,
+                 PERF_FLAG_FD_CLOEXEC);
+}
+
 static int bpf_attach_tracing_event(int progfd, const char *event_path,
-    struct perf_reader *reader, int pid, int cpu, int group_fd) {
-  int efd, pfd;
+    struct perf_reader *reader, int pid, int cpu, int group_fd, int pfd) {
+  int efd;
   ssize_t bytes;
   char buf[256];
   struct perf_event_attr attr = {};
 
-  snprintf(buf, sizeof(buf), "%s/id", event_path);
-  efd = open(buf, O_RDONLY, 0);
-  if (efd < 0) {
-    fprintf(stderr, "open(%s): %s\n", buf, strerror(errno));
-    return -1;
-  }
+  /*
+   * Only look up id and call perf_event_open when
+   * bpf_try_perf_event_open_with_probe() didn't returns valid pfd.
+   */
+  if (pfd < 0) {
+    snprintf(buf, sizeof(buf), "%s/id", event_path);
+    efd = open(buf, O_RDONLY, 0);
+    if (efd < 0) {
+      fprintf(stderr, "open(%s): %s\n", buf, strerror(errno));
+      return -1;
+    }
 
-  bytes = read(efd, buf, sizeof(buf));
-  if (bytes <= 0 || bytes >= sizeof(buf)) {
-    fprintf(stderr, "read(%s): %s\n", buf, strerror(errno));
+    bytes = read(efd, buf, sizeof(buf));
+    if (bytes <= 0 || bytes >= sizeof(buf)) {
+      fprintf(stderr, "read(%s): %s\n", buf, strerror(errno));
+      close(efd);
+      return -1;
+    }
     close(efd);
-    return -1;
-  }
-  close(efd);
-  buf[bytes] = '\0';
-  attr.config = strtol(buf, NULL, 0);
-  attr.type = PERF_TYPE_TRACEPOINT;
-  attr.sample_type = PERF_SAMPLE_RAW | PERF_SAMPLE_CALLCHAIN;
-  attr.sample_period = 1;
-  attr.wakeup_events = 1;
-  pfd = syscall(__NR_perf_event_open, &attr, pid, cpu, group_fd, PERF_FLAG_FD_CLOEXEC);
-  if (pfd < 0) {
-    fprintf(stderr, "perf_event_open(%s/id): %s\n", event_path, strerror(errno));
-    return -1;
+    buf[bytes] = '\0';
+    attr.config = strtol(buf, NULL, 0);
+    attr.type = PERF_TYPE_TRACEPOINT;
+    attr.sample_type = PERF_SAMPLE_RAW | PERF_SAMPLE_CALLCHAIN;
+    attr.sample_period = 1;
+    attr.wakeup_events = 1;
+    pfd = syscall(__NR_perf_event_open, &attr, pid, cpu, group_fd, PERF_FLAG_FD_CLOEXEC);
+    if (pfd < 0) {
+      fprintf(stderr, "perf_event_open(%s/id): %s\n", event_path, strerror(errno));
+      return -1;
+    }
   }
+
   perf_reader_set_fd(reader, pfd);
 
   if (perf_reader_mmap(reader, attr.type, attr.sample_type) < 0)
@@ -575,6 +604,26 @@ static int bpf_attach_tracing_event(int progfd, const char *event_path,
   return 0;
 }
 
+#define PMU_TYPE_FILE "/sys/bus/event_source/devices/%s/type"
+static int bpf_find_probe_type(const char *p_type)
+{
+  int fd;
+  int ret;
+  char buf[64];
+
+  snprintf(buf, sizeof(buf), PMU_TYPE_FILE, p_type);
+
+  fd = open(buf, O_RDONLY);
+  if (fd < 0)
+    return -1;
+  ret = read(fd, buf, sizeof(buf));
+  close(fd);
+  if (ret < 0 || ret >= sizeof(buf))
+    return -1;
+  ret = (int)strtol(buf, NULL, 10);
+  return errno ? -1 : ret;
+}
+
 void * bpf_attach_kprobe(int progfd, enum bpf_probe_attach_type attach_type, const char *ev_name,
                         const char *fn_name,
                         pid_t pid, int cpu, int group_fd,
@@ -585,31 +634,42 @@ void * bpf_attach_kprobe(int progfd, enum bpf_probe_attach_type attach_type, con
   char event_alias[128];
   struct perf_reader *reader = NULL;
   static char *event_type = "kprobe";
+  int pfd = -1;
+  int perf_probe_type;
 
   reader = perf_reader_new(cb, NULL, NULL, cb_cookie, probe_perf_reader_page_cnt);
   if (!reader)
     goto error;
 
-  snprintf(buf, sizeof(buf), "/sys/kernel/debug/tracing/%s_events", event_type);
-  kfd = open(buf, O_WRONLY | O_APPEND, 0);
-  if (kfd < 0) {
-    fprintf(stderr, "open(%s): %s\n", buf, strerror(errno));
-    goto error;
-  }
+  /* try use new API to create kprobe */
+  perf_probe_type = bpf_find_probe_type(event_type);
+  if (perf_probe_type != -1)
+    pfd = bpf_try_perf_event_open_with_probe(fn_name, 0, pid, cpu, group_fd,
+                                             perf_probe_type,
+                                             attach_type != BPF_PROBE_ENTRY);
 
-  snprintf(event_alias, sizeof(event_alias), "%s_bcc_%d", ev_name, getpid());
-  snprintf(buf, sizeof(buf), "%c:%ss/%s %s", attach_type==BPF_PROBE_ENTRY ? 'p' : 'r',
-			event_type, event_alias, fn_name);
-  if (write(kfd, buf, strlen(buf)) < 0) {
-    if (errno == EINVAL)
-      fprintf(stderr, "check dmesg output for possible cause\n");
+  if (pfd < 0) {
+    snprintf(buf, sizeof(buf), "/sys/kernel/debug/tracing/%s_events", event_type);
+    kfd = open(buf, O_WRONLY | O_APPEND, 0);
+    if (kfd < 0) {
+      fprintf(stderr, "open(%s): %s\n", buf, strerror(errno));
+      goto error;
+    }
+
+    snprintf(event_alias, sizeof(event_alias), "%s_bcc_%d", ev_name, getpid());
+    snprintf(buf, sizeof(buf), "%c:%ss/%s %s", attach_type==BPF_PROBE_ENTRY ? 'p' : 'r',
+             event_type, event_alias, fn_name);
+    if (write(kfd, buf, strlen(buf)) < 0) {
+      if (errno == EINVAL)
+        fprintf(stderr, "check dmesg output for possible cause\n");
+      close(kfd);
+      goto error;
+    }
     close(kfd);
-    goto error;
+    snprintf(buf, sizeof(buf), "/sys/kernel/debug/tracing/events/%ss/%s", event_type, event_alias);
   }
-  close(kfd);
 
-  snprintf(buf, sizeof(buf), "/sys/kernel/debug/tracing/events/%ss/%s", event_type, event_alias);
-  if (bpf_attach_tracing_event(progfd, buf, reader, pid, cpu, group_fd) < 0)
+  if (bpf_attach_tracing_event(progfd, buf, reader, pid, cpu, group_fd, pfd) < 0)
     goto error;
 
   return reader;
@@ -691,42 +751,52 @@ void * bpf_attach_uprobe(int progfd, enum bpf_probe_attach_type attach_type, con
   struct perf_reader *reader = NULL;
   static char *event_type = "uprobe";
   int res, kfd = -1, ns_fd = -1;
+  int pfd = -1;
+  int perf_probe_type;
 
   reader = perf_reader_new(cb, NULL, NULL, cb_cookie, probe_perf_reader_page_cnt);
   if (!reader)
     goto error;
 
-  snprintf(buf, sizeof(buf), "/sys/kernel/debug/tracing/%s_events", event_type);
-  kfd = open(buf, O_WRONLY | O_APPEND, 0);
-  if (kfd < 0) {
-    fprintf(stderr, "open(%s): %s\n", buf, strerror(errno));
-    goto error;
-  }
+  /* try use new API to create uprobe */
+  perf_probe_type = bpf_find_probe_type(event_type);
+  pfd = bpf_try_perf_event_open_with_probe(binary_path, offset, pid, cpu,
+            group_fd, perf_probe_type, attach_type != BPF_PROBE_ENTRY);
 
-  res = snprintf(event_alias, sizeof(event_alias), "%s_bcc_%d", ev_name, getpid());
-  if (res < 0 || res >= sizeof(event_alias)) {
-    fprintf(stderr, "Event name (%s) is too long for buffer\n", ev_name);
-    goto error;
-  }
-  res = snprintf(buf, sizeof(buf), "%c:%ss/%s %s:0x%lx", attach_type==BPF_PROBE_ENTRY ? 'p' : 'r',
-			event_type, event_alias, binary_path, offset);
-  if (res < 0 || res >= sizeof(buf)) {
-    fprintf(stderr, "Event alias (%s) too long for buffer\n", event_alias);
-    goto error;
-  }
+  if (pfd < 0) {
+    snprintf(buf, sizeof(buf), "/sys/kernel/debug/tracing/%s_events", event_type);
+    kfd = open(buf, O_WRONLY | O_APPEND, 0);
+    if (kfd < 0) {
+      fprintf(stderr, "open(%s): %s\n", buf, strerror(errno));
+      goto error;
+    }
 
-  ns_fd = enter_mount_ns(pid);
-  if (write(kfd, buf, strlen(buf)) < 0) {
-    if (errno == EINVAL)
-      fprintf(stderr, "check dmesg output for possible cause\n");
-    goto error;
+    res = snprintf(event_alias, sizeof(event_alias), "%s_bcc_%d", ev_name, getpid());
+    if (res < 0 || res >= sizeof(event_alias)) {
+      fprintf(stderr, "Event name (%s) is too long for buffer\n", ev_name);
+      goto error;
+    }
+    res = snprintf(buf, sizeof(buf), "%c:%ss/%s %s:0x%lx", attach_type==BPF_PROBE_ENTRY ? 'p' : 'r',
+                   event_type, event_alias, binary_path, offset);
+    if (res < 0 || res >= sizeof(buf)) {
+      fprintf(stderr, "Event alias (%s) too long for buffer\n", event_alias);
+      goto error;
+    }
+
+    ns_fd = enter_mount_ns(pid);
+    if (write(kfd, buf, strlen(buf)) < 0) {
+      if (errno == EINVAL)
+        fprintf(stderr, "check dmesg output for possible cause\n");
+      goto error;
+    }
+    close(kfd);
+    exit_mount_ns(ns_fd);
+    ns_fd = -1;
+
+    snprintf(buf, sizeof(buf), "/sys/kernel/debug/tracing/events/%ss/%s", event_type, event_alias);
   }
-  close(kfd);
-  exit_mount_ns(ns_fd);
-  ns_fd = -1;
 
-  snprintf(buf, sizeof(buf), "/sys/kernel/debug/tracing/events/%ss/%s", event_type, event_alias);
-  if (bpf_attach_tracing_event(progfd, buf, reader, pid, cpu, group_fd) < 0)
+  if (bpf_attach_tracing_event(progfd, buf, reader, pid, cpu, group_fd, pfd) < 0)
     goto error;
 
   return reader;
@@ -741,8 +811,43 @@ error:
 
 static int bpf_detach_probe(const char *ev_name, const char *event_type)
 {
-  int kfd, res;
+  int kfd = -1, res;
   char buf[PATH_MAX];
+  int found_event = 0;
+  size_t bufsize = 0;
+  char *cptr = NULL;
+  FILE *fp;
+
+  /*
+   * For [k,u]probe created with perf_event_open (on newer kernel), it is
+   * not necessary to clean it up in [k,u]probe_events. We first look up
+   * the %s_bcc_%d line in [k,u]probe_events. If the event is not found,
+   * it is safe to skip the cleaning up process (write -:... to the file).
+   */
+  snprintf(buf, sizeof(buf), "/sys/kernel/debug/tracing/%s_events", event_type);
+  fp = fopen(buf, "r");
+  if (!fp) {
+    fprintf(stderr, "open(%s): %s\n", buf, strerror(errno));
+    goto error;
+  }
+
+  res = snprintf(buf, sizeof(buf), "%ss/%s_bcc_%d", event_type, ev_name, getpid());
+  if (res < 0 || res >= sizeof(buf)) {
+    fprintf(stderr, "snprintf(%s): %d\n", ev_name, res);
+    goto error;
+  }
+
+  while (getline(&cptr, &bufsize, fp) != -1)
+    if (strstr(cptr, buf) != NULL) {
+      found_event = 1;
+      break;
+    }
+  fclose(fp);
+  fp = NULL;
+
+  if (!found_event)
+    return 0;
+
   snprintf(buf, sizeof(buf), "/sys/kernel/debug/tracing/%s_events", event_type);
   kfd = open(buf, O_WRONLY | O_APPEND, 0);
   if (kfd < 0) {
@@ -766,6 +871,8 @@ static int bpf_detach_probe(const char *ev_name, const char *event_type)
 error:
   if (kfd >= 0)
     close(kfd);
+  if (fp)
+    fclose(fp);
   return -1;
 }
 
@@ -792,7 +899,7 @@ void * bpf_attach_tracepoint(int progfd, const char *tp_category,
 
   snprintf(buf, sizeof(buf), "/sys/kernel/debug/tracing/events/%s/%s",
            tp_category, tp_name);
-  if (bpf_attach_tracing_event(progfd, buf, reader, pid, cpu, group_fd) < 0)
+  if (bpf_attach_tracing_event(progfd, buf, reader, pid, cpu, group_fd, -1) < 0)
     goto error;
 
   return reader;
-- 
2.9.5

^ permalink raw reply related

* [PATCH v3 1/6] perf: prepare perf_event.h for new types perf_kprobe and perf_uprobe
From: Song Liu @ 2017-11-30 23:50 UTC (permalink / raw)
  To: peterz, rostedt, mingo, davem, netdev, linux-kernel, daniel
  Cc: kernel-team, Song Liu
In-Reply-To: <20171130235023.1414663-1-songliubraving@fb.com>

Two new perf types, perf_kprobe and perf_uprobe, will be added to allow
creating [k,u]probe with perf_event_open. These [k,u]probe are associated
with the file decriptor created by perf_event_open, thus are easy to
clean when the file descriptor is destroyed.

kprobe_func and uprobe_path are added to union config1 for pointers to
function name for kprobe or binary path for uprobe.

kprobe_addr and probe_offset are added to union config2 for kernel
address (when kprobe_func is NULL), or [k,u]probe offset.

Signed-off-by: Song Liu <songliubraving@fb.com>
Reviewed-by: Yonghong Song <yhs@fb.com>
Reviewed-by: Josef Bacik <jbacik@fb.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 include/uapi/linux/perf_event.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 362493a..247c6cb 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -299,6 +299,8 @@ enum perf_event_read_format {
 #define PERF_ATTR_SIZE_VER4	104	/* add: sample_regs_intr */
 #define PERF_ATTR_SIZE_VER5	112	/* add: aux_watermark */
 
+#define MAX_PROBE_FUNC_NAME_LEN 64
+
 /*
  * Hardware event_id to monitor via a performance monitoring event:
  *
@@ -380,10 +382,14 @@ struct perf_event_attr {
 	__u32			bp_type;
 	union {
 		__u64		bp_addr;
+		__u64		kprobe_func; /* for perf_kprobe */
+		__u64		uprobe_path; /* for perf_uprobe */
 		__u64		config1; /* extension of config */
 	};
 	union {
 		__u64		bp_len;
+		__u64		kprobe_addr; /* when kprobe_func == NULL */
+		__u64		probe_offset; /* for perf_[k,u]probe */
 		__u64		config2; /* extension of config1 */
 	};
 	__u64	branch_sample_type; /* enum perf_branch_sample_type */
-- 
2.9.5

^ permalink raw reply related

* [PATCH v3 4/6] perf: implement pmu perf_uprobe
From: Song Liu @ 2017-11-30 23:50 UTC (permalink / raw)
  To: peterz, rostedt, mingo, davem, netdev, linux-kernel, daniel
  Cc: kernel-team, Song Liu
In-Reply-To: <20171130235023.1414663-1-songliubraving@fb.com>

This patch adds perf_uprobe support with similar pattern as previous
patch (for kprobe).

Two functions, create_local_trace_uprobe() and
destroy_local_trace_uprobe(), are created so a uprobe can be created
and attached to the file descriptor created by perf_event_open().

Signed-off-by: Song Liu <songliubraving@fb.com>
Reviewed-by: Yonghong Song <yhs@fb.com>
Reviewed-by: Josef Bacik <jbacik@fb.com>
---
 include/linux/trace_events.h    |  2 +
 kernel/events/core.c            | 36 ++++++++++++++++-
 kernel/trace/trace_event_perf.c | 58 +++++++++++++++++++++++++++
 kernel/trace/trace_probe.h      |  4 ++
 kernel/trace/trace_uprobe.c     | 86 +++++++++++++++++++++++++++++++++++++----
 5 files changed, 177 insertions(+), 9 deletions(-)

diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 51f748c9..9272fa6 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -496,6 +496,8 @@ extern int  perf_trace_add(struct perf_event *event, int flags);
 extern void perf_trace_del(struct perf_event *event, int flags);
 extern int  perf_kprobe_init(struct perf_event *event);
 extern void perf_kprobe_destroy(struct perf_event *event);
+extern int  perf_uprobe_init(struct perf_event *event);
+extern void perf_uprobe_destroy(struct perf_event *event);
 extern int  ftrace_profile_set_filter(struct perf_event *event, int event_id,
 				     char *filter_str);
 extern void ftrace_profile_free_filter(struct perf_event *event);
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 49bbf46..fcdadb3 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8013,10 +8013,43 @@ static int perf_kprobe_event_init(struct perf_event *event)
 	return 0;
 }
 
+static int perf_uprobe_event_init(struct perf_event *event);
+static struct pmu perf_uprobe = {
+	.task_ctx_nr	= perf_sw_context,
+	.event_init	= perf_uprobe_event_init,
+	.add		= perf_trace_add,
+	.del		= perf_trace_del,
+	.start		= perf_swevent_start,
+	.stop		= perf_swevent_stop,
+	.read		= perf_swevent_read,
+};
+
+static int perf_uprobe_event_init(struct perf_event *event)
+{
+	int err;
+
+	if (event->attr.type != perf_uprobe.type)
+		return -ENOENT;
+	/*
+	 * no branch sampling for probe events
+	 */
+	if (has_branch_stack(event))
+		return -EOPNOTSUPP;
+
+	err = perf_uprobe_init(event);
+	if (err)
+		return err;
+
+	event->destroy = perf_uprobe_destroy;
+
+	return 0;
+}
+
 static inline void perf_tp_register(void)
 {
 	perf_pmu_register(&perf_tracepoint, "tracepoint", PERF_TYPE_TRACEPOINT);
 	perf_pmu_register(&perf_kprobe, "kprobe", -1);
+	perf_pmu_register(&perf_uprobe, "uprobe", -1);
 }
 
 static void perf_event_free_filter(struct perf_event *event)
@@ -8100,7 +8133,8 @@ static void perf_event_free_bpf_handler(struct perf_event *event)
 static inline bool perf_event_is_tracing(struct perf_event *event)
 {
 	return event->attr.type == PERF_TYPE_TRACEPOINT ||
-		strncmp(event->pmu->name, "kprobe", 6) == 0;
+		strncmp(event->pmu->name, "kprobe", 6) == 0 ||
+		strncmp(event->pmu->name, "uprobe", 6) == 0;
 }
 
 static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd)
diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 7cf0d99..1b97ea2 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -272,6 +272,52 @@ int perf_kprobe_init(struct perf_event *p_event)
 #endif /* CONFIG_KPROBE_EVENTS */
 }
 
+int perf_uprobe_init(struct perf_event *p_event)
+{
+	int ret;
+	char *path = NULL;
+	struct trace_event_call *tp_event;
+
+#ifdef CONFIG_UPROBE_EVENTS
+	if (!p_event->attr.uprobe_path)
+		return -EINVAL;
+	path = kzalloc(PATH_MAX, GFP_KERNEL);
+	if (!path)
+		return -ENOMEM;
+	ret = strncpy_from_user(
+		path, u64_to_user_ptr(p_event->attr.uprobe_path), PATH_MAX);
+	if (ret < 0)
+		goto out;
+	if (path[0] == '\0') {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	tp_event = create_local_trace_uprobe(
+		path, p_event->attr.probe_offset, p_event->attr.config != 0);
+	if (IS_ERR(tp_event)) {
+		ret = PTR_ERR(tp_event);
+		goto out;
+	}
+
+	/*
+	 * local trace_uprobe need to hold event_mutex to call
+	 * uprobe_buffer_enable() and uprobe_buffer_disable().
+	 * event_mutex is not required for local trace_kprobes.
+	 */
+	mutex_lock(&event_mutex);
+	ret = perf_trace_event_init(tp_event, p_event);
+	if (ret)
+		destroy_local_trace_uprobe(tp_event);
+	mutex_unlock(&event_mutex);
+out:
+	kfree(path);
+	return ret;
+#else
+	return -EOPNOTSUPP;
+#endif /* CONFIG_UPROBE_EVENTS */
+}
+
 void perf_trace_destroy(struct perf_event *p_event)
 {
 	mutex_lock(&event_mutex);
@@ -290,6 +336,18 @@ void perf_kprobe_destroy(struct perf_event *p_event)
 #endif
 }
 
+void perf_uprobe_destroy(struct perf_event *p_event)
+{
+	mutex_lock(&event_mutex);
+	perf_trace_event_close(p_event);
+	perf_trace_event_unreg(p_event);
+	mutex_unlock(&event_mutex);
+
+#ifdef CONFIG_UPROBE_EVENTS
+	destroy_local_trace_uprobe(p_event->tp_event);
+#endif
+}
+
 int perf_trace_add(struct perf_event *p_event, int flags)
 {
 	struct trace_event_call *tp_event = p_event->tp_event;
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 910ae1b..86b5925 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -417,4 +417,8 @@ extern struct trace_event_call *
 create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
 			  bool is_return);
 extern void destroy_local_trace_kprobe(struct trace_event_call *event_call);
+
+extern struct trace_event_call *
+create_local_trace_uprobe(char *name, unsigned long offs, bool is_return);
+extern void destroy_local_trace_uprobe(struct trace_event_call *event_call);
 #endif
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 4525e02..4d805d2 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1293,16 +1293,25 @@ static struct trace_event_functions uprobe_funcs = {
 	.trace		= print_uprobe_event
 };
 
-static int register_uprobe_event(struct trace_uprobe *tu)
+static inline void init_trace_event_call(struct trace_uprobe *tu,
+					 struct trace_event_call *call)
 {
-	struct trace_event_call *call = &tu->tp.call;
-	int ret;
-
-	/* Initialize trace_event_call */
 	INIT_LIST_HEAD(&call->class->fields);
 	call->event.funcs = &uprobe_funcs;
 	call->class->define_fields = uprobe_event_define_fields;
 
+	call->flags = TRACE_EVENT_FL_UPROBE;
+	call->class->reg = trace_uprobe_register;
+	call->data = tu;
+}
+
+static int register_uprobe_event(struct trace_uprobe *tu)
+{
+	struct trace_event_call *call = &tu->tp.call;
+	int ret = 0;
+
+	init_trace_event_call(tu, call);
+
 	if (set_print_fmt(&tu->tp, is_ret_probe(tu)) < 0)
 		return -ENOMEM;
 
@@ -1312,9 +1321,6 @@ static int register_uprobe_event(struct trace_uprobe *tu)
 		return -ENODEV;
 	}
 
-	call->flags = TRACE_EVENT_FL_UPROBE;
-	call->class->reg = trace_uprobe_register;
-	call->data = tu;
 	ret = trace_add_event_call(call);
 
 	if (ret) {
@@ -1340,6 +1346,70 @@ static int unregister_uprobe_event(struct trace_uprobe *tu)
 	return 0;
 }
 
+#ifdef CONFIG_PERF_EVENTS
+struct trace_event_call *
+create_local_trace_uprobe(char *name, unsigned long offs, bool is_return)
+{
+	struct trace_uprobe *tu;
+	struct inode *inode;
+	struct path path;
+	int ret;
+
+	ret = kern_path(name, LOOKUP_FOLLOW, &path);
+	if (ret)
+		return ERR_PTR(ret);
+
+	inode = igrab(d_inode(path.dentry));
+	path_put(&path);
+
+	if (!inode || !S_ISREG(inode->i_mode)) {
+		iput(inode);
+		return ERR_PTR(-EINVAL);
+	}
+
+	/*
+	 * local trace_kprobes are not added to probe_list, so they are never
+	 * searched in find_trace_kprobe(). Therefore, there is no concern of
+	 * duplicated name "DUMMY_EVENT" here.
+	 */
+	tu = alloc_trace_uprobe(UPROBE_EVENT_SYSTEM, "DUMMY_EVENT", 0,
+				is_return);
+
+	if (IS_ERR(tu)) {
+		pr_info("Failed to allocate trace_uprobe.(%d)\n",
+			(int)PTR_ERR(tu));
+		return ERR_CAST(tu);
+	}
+
+	tu->offset = offs;
+	tu->inode = inode;
+	tu->filename = kstrdup(name, GFP_KERNEL);
+	init_trace_event_call(tu, &tu->tp.call);
+
+	if (set_print_fmt(&tu->tp, is_ret_probe(tu)) < 0) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	return &tu->tp.call;
+error:
+	free_trace_uprobe(tu);
+	return ERR_PTR(ret);
+}
+
+void destroy_local_trace_uprobe(struct trace_event_call *event_call)
+{
+	struct trace_uprobe *tu;
+
+	tu = container_of(event_call, struct trace_uprobe, tp.call);
+
+	kfree(tu->tp.call.print_fmt);
+	tu->tp.call.print_fmt = NULL;
+
+	free_trace_uprobe(tu);
+}
+#endif /* CONFIG_PERF_EVENTS */
+
 /* Make a trace interface for controling probe points */
 static __init int init_uprobe_trace(void)
 {
-- 
2.9.5

^ 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