[Patch net-next v2] pktgen: support net namespace

[Patch net-next v2] pktgen: support net namespace

[Cong Wang]

From: Cong Wang <amwang@redhat.com>

v2: remove a useless check

This patch add net namespace to pktgen, so that
we can use pktgen in different namespaces.

Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>

---
 net/core/pktgen.c |  123 +++++++++++++++++++++++++++++++++++------------------
 1 files changed, 81 insertions(+), 42 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index b29dacf..c217033 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -164,6 +164,7 @@
 #ifdef CONFIG_XFRM
 #include <net/xfrm.h>
 #endif
+#include <net/netns/generic.h>
 #include <asm/byteorder.h>
 #include <linux/rcupdate.h>
 #include <linux/bitops.h>
@@ -212,7 +213,6 @@
 #define PKTGEN_MAGIC 0xbe9be955
 #define PG_PROC_DIR "pktgen"
 #define PGCTRL	    "pgctrl"
-static struct proc_dir_entry *pg_proc_dir;
 
 #define MAX_CFLOWS  65536
 
@@ -414,6 +414,15 @@ struct pktgen_thread {
 
 	wait_queue_head_t queue;
 	struct completion start_done;
+	struct pktgen_net *net;
+};
+
+static int pg_net_id __read_mostly;
+
+struct pktgen_net {
+	struct net		*net;
+	struct net_device	*dev;
+	struct proc_dir_entry	*proc_dir;
 };
 
 #define REMOVE 1
@@ -1886,15 +1895,16 @@ static void pktgen_change_name(struct net_device *dev)
 
 	list_for_each_entry(t, &pktgen_threads, th_list) {
 		struct pktgen_dev *pkt_dev;
+		struct pktgen_net *pn = t->net;
 
 		list_for_each_entry(pkt_dev, &t->if_list, list) {
 			if (pkt_dev->odev != dev)
 				continue;
 
-			remove_proc_entry(pkt_dev->entry->name, pg_proc_dir);
+			remove_proc_entry(pkt_dev->entry->name, pn->proc_dir);
 
 			pkt_dev->entry = proc_create_data(dev->name, 0600,
-							  pg_proc_dir,
+							  pn->proc_dir,
 							  &pktgen_if_fops,
 							  pkt_dev);
 			if (!pkt_dev->entry)
@@ -1909,8 +1919,9 @@ static int pktgen_device_event(struct notifier_block *unused,
 			       unsigned long event, void *ptr)
 {
 	struct net_device *dev = ptr;
+	struct pktgen_net *pn = net_generic(dev_net(dev), pg_net_id);
 
-	if (!net_eq(dev_net(dev), &init_net) || pktgen_exiting)
+	if (pn->dev != dev || pktgen_exiting)
 		return NOTIFY_DONE;
 
 	/* It is OK that we do not hold the group lock right now,
@@ -1930,7 +1941,8 @@ static int pktgen_device_event(struct notifier_block *unused,
 	return NOTIFY_DONE;
 }
 
-static struct net_device *pktgen_dev_get_by_name(struct pktgen_dev *pkt_dev,
+static struct net_device *pktgen_dev_get_by_name(const struct pktgen_net *pn,
+						 struct pktgen_dev *pkt_dev,
 						 const char *ifname)
 {
 	char b[IFNAMSIZ+5];
@@ -1944,13 +1956,14 @@ static struct net_device *pktgen_dev_get_by_name(struct pktgen_dev *pkt_dev,
 	}
 	b[i] = 0;
 
-	return dev_get_by_name(&init_net, b);
+	return dev_get_by_name(pn->net, b);
 }
 
 
 /* Associate pktgen_dev with a device. */
 
-static int pktgen_setup_dev(struct pktgen_dev *pkt_dev, const char *ifname)
+static int pktgen_setup_dev(const struct pktgen_net *pn,
+			    struct pktgen_dev *pkt_dev, const char *ifname)
 {
 	struct net_device *odev;
 	int err;
@@ -1961,7 +1974,7 @@ static int pktgen_setup_dev(struct pktgen_dev *pkt_dev, const char *ifname)
 		pkt_dev->odev = NULL;
 	}
 
-	odev = pktgen_dev_get_by_name(pkt_dev, ifname);
+	odev = pktgen_dev_get_by_name(pn, pkt_dev, ifname);
 	if (!odev) {
 		pr_err("no such netdevice: \"%s\"\n", ifname);
 		return -ENODEV;
@@ -2203,9 +2216,10 @@ static inline int f_pick(struct pktgen_dev *pkt_dev)
 static void get_ipsec_sa(struct pktgen_dev *pkt_dev, int flow)
 {
 	struct xfrm_state *x = pkt_dev->flows[flow].x;
+	struct pktgen_net *pn = net_generic(dev_net(pkt_dev->odev), pg_net_id);
 	if (!x) {
 		/*slow path: we dont already have xfrm_state*/
-		x = xfrm_stateonly_find(&init_net, DUMMY_MARK,
+		x = xfrm_stateonly_find(pn->net, DUMMY_MARK,
 					(xfrm_address_t *)&pkt_dev->cur_daddr,
 					(xfrm_address_t *)&pkt_dev->cur_saddr,
 					AF_INET,
@@ -3154,9 +3168,7 @@ static void pktgen_rem_all_ifs(struct pktgen_thread *t)
 static void pktgen_rem_thread(struct pktgen_thread *t)
 {
 	/* Remove from the thread list */
-
-	remove_proc_entry(t->tsk->comm, pg_proc_dir);
-
+	remove_proc_entry(t->tsk->comm, t->net->proc_dir);
 }
 
 static void pktgen_resched(struct pktgen_dev *pkt_dev)
@@ -3459,13 +3471,14 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 	pkt_dev->svlan_id = 0xffff;
 	pkt_dev->node = -1;
 
-	err = pktgen_setup_dev(pkt_dev, ifname);
+	err = pktgen_setup_dev(t->net, pkt_dev, ifname);
 	if (err)
 		goto out1;
 	if (pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)
 		pkt_dev->clone_skb = pg_clone_skb_d;
 
-	pkt_dev->entry = proc_create_data(ifname, 0600, pg_proc_dir,
+	t->net->dev = pkt_dev->odev;
+	pkt_dev->entry = proc_create_data(ifname, 0600, t->net->proc_dir,
 					  &pktgen_if_fops, pkt_dev);
 	if (!pkt_dev->entry) {
 		pr_err("cannot create %s/%s procfs entry\n",
@@ -3490,7 +3503,7 @@ out1:
 	return err;
 }
 
-static int __init pktgen_create_thread(int cpu)
+static int __net_init pktgen_create_thread(int cpu, struct pktgen_net *pn)
 {
 	struct pktgen_thread *t;
 	struct proc_dir_entry *pe;
@@ -3524,7 +3537,7 @@ static int __init pktgen_create_thread(int cpu)
 	kthread_bind(p, cpu);
 	t->tsk = p;
 
-	pe = proc_create_data(t->tsk->comm, 0600, pg_proc_dir,
+	pe = proc_create_data(t->tsk->comm, 0600, pn->proc_dir,
 			      &pktgen_thread_fops, t);
 	if (!pe) {
 		pr_err("cannot create %s/%s procfs entry\n",
@@ -3535,6 +3548,7 @@ static int __init pktgen_create_thread(int cpu)
 		return -EINVAL;
 	}
 
+	t->net = pn;
 	wake_up_process(p);
 	wait_for_completion(&t->start_done);
 
@@ -3560,6 +3574,7 @@ static void _rem_dev_from_if_list(struct pktgen_thread *t,
 static int pktgen_remove_device(struct pktgen_thread *t,
 				struct pktgen_dev *pkt_dev)
 {
+	struct pktgen_net *pn = t->net;
 
 	pr_debug("remove_device pkt_dev=%p\n", pkt_dev);
 
@@ -3580,7 +3595,7 @@ static int pktgen_remove_device(struct pktgen_thread *t,
 	_rem_dev_from_if_list(t, pkt_dev);
 
 	if (pkt_dev->entry)
-		remove_proc_entry(pkt_dev->entry->name, pg_proc_dir);
+		remove_proc_entry(pkt_dev->entry->name, pn->proc_dir);
 
 #ifdef CONFIG_XFRM
 	free_SAs(pkt_dev);
@@ -3592,49 +3607,75 @@ static int pktgen_remove_device(struct pktgen_thread *t,
 	return 0;
 }
 
-static int __init pg_init(void)
+static int __net_init pg_net_init(struct net *net)
 {
-	int cpu;
+	struct pktgen_net *pn = net_generic(net, pg_net_id);
 	struct proc_dir_entry *pe;
-	int ret = 0;
-
-	pr_info("%s", version);
+	int cpu, ret = 0;
 
-	pg_proc_dir = proc_mkdir(PG_PROC_DIR, init_net.proc_net);
-	if (!pg_proc_dir)
+	pn->net = net;
+	pn->proc_dir = proc_mkdir(PG_PROC_DIR, pn->net->proc_net);
+	if (!pn->proc_dir) {
+		pr_warn("cannot create /proc/net/%s\n", PG_PROC_DIR);
 		return -ENODEV;
-
-	pe = proc_create(PGCTRL, 0600, pg_proc_dir, &pktgen_fops);
+	}
+	pe = proc_create(PGCTRL, 0600, pn->proc_dir, &pktgen_fops);
 	if (pe == NULL) {
-		pr_err("ERROR: cannot create %s procfs entry\n", PGCTRL);
+		pr_err("cannot create %s procfs entry\n", PGCTRL);
 		ret = -EINVAL;
-		goto remove_dir;
+		goto remove;
 	}
 
-	register_netdevice_notifier(&pktgen_notifier_block);
-
 	for_each_online_cpu(cpu) {
 		int err;
 
-		err = pktgen_create_thread(cpu);
+		err = pktgen_create_thread(cpu, pn);
 		if (err)
-			pr_warning("WARNING: Cannot create thread for cpu %d (%d)\n",
+			pr_warn("Cannot create thread for cpu %d (%d)\n",
 				   cpu, err);
 	}
 
 	if (list_empty(&pktgen_threads)) {
-		pr_err("ERROR: Initialization failed for all threads\n");
+		pr_err("Initialization failed for all threads\n");
 		ret = -ENODEV;
-		goto unregister;
+		goto remove_entry;
 	}
 
 	return 0;
 
- unregister:
-	unregister_netdevice_notifier(&pktgen_notifier_block);
-	remove_proc_entry(PGCTRL, pg_proc_dir);
- remove_dir:
-	proc_net_remove(&init_net, PG_PROC_DIR);
+remove_entry:
+	remove_proc_entry(PGCTRL, pn->proc_dir);
+remove:
+	proc_net_remove(pn->net, PG_PROC_DIR);
+	return ret;
+}
+
+static void __net_exit pg_net_exit(struct net *net)
+{
+	struct pktgen_net *pn = net_generic(net, pg_net_id);
+	remove_proc_entry(PGCTRL, pn->proc_dir);
+	proc_net_remove(pn->net, PG_PROC_DIR);
+}
+
+static struct pernet_operations pg_net_ops = {
+	.init = pg_net_init,
+	.exit = pg_net_exit,
+	.id   = &pg_net_id,
+	.size = sizeof(struct pktgen_net),
+};
+
+static int __init pg_init(void)
+{
+	int ret = 0;
+
+	pr_info("%s", version);
+	ret = register_pernet_subsys(&pg_net_ops);
+	if (ret)
+		return ret;
+	ret = register_netdevice_notifier(&pktgen_notifier_block);
+	if (ret)
+		unregister_pernet_subsys(&pg_net_ops);
+
 	return ret;
 }
 
@@ -3661,9 +3702,7 @@ static void __exit pg_cleanup(void)
 	/* Un-register us from receiving netdevice events */
 	unregister_netdevice_notifier(&pktgen_notifier_block);
 
-	/* Clean up proc file system */
-	remove_proc_entry(PGCTRL, pg_proc_dir);
-	proc_net_remove(&init_net, PG_PROC_DIR);
+	unregister_pernet_subsys(&pg_net_ops);
 }
 
 module_init(pg_init);

Re: [Patch net-next v2] pktgen: support net namespace

[Eric W. Biederman]

Cong Wang <amwang@redhat.com> writes:

> From: Cong Wang <amwang@redhat.com>
>
> v2: remove a useless check
>
> This patch add net namespace to pktgen, so that
> we can use pktgen in different namespaces.
>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
>
> ---
>  net/core/pktgen.c |  123 +++++++++++++++++++++++++++++++++++------------------
>  1 files changed, 81 insertions(+), 42 deletions(-)

Skiming through this again I have spotted what looks like a pretty
major bug.  You are limiting yourself to one network device per network
namespace when the actual limit is one network device per thread.

I think you can just kill the dev member of pktgen_net and the two or
three lines of code that touch it.

> +static int pg_net_id __read_mostly;
> +
> +struct pktgen_net {
> +	struct net		*net;
> +	struct net_device	*dev;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +	struct proc_dir_entry	*proc_dir;
>  };

Eric

Re: [Patch net-next v2] pktgen: support net namespace

[Cong Wang]

On Mon, 2013-01-28 at 18:36 -0800, Eric W. Biederman wrote:
> Cong Wang <amwang@redhat.com> writes:
> 
> > From: Cong Wang <amwang@redhat.com>
> >
> > v2: remove a useless check
> >
> > This patch add net namespace to pktgen, so that
> > we can use pktgen in different namespaces.
> >
> > Cc: Eric W. Biederman <ebiederm@xmission.com>
> > Cc: David S. Miller <davem@davemloft.net>
> > Signed-off-by: Cong Wang <amwang@redhat.com>
> >
> > ---
> >  net/core/pktgen.c |  123 +++++++++++++++++++++++++++++++++++------------------
> >  1 files changed, 81 insertions(+), 42 deletions(-)
> 
> Skiming through this again I have spotted what looks like a pretty
> major bug.  You are limiting yourself to one network device per network
> namespace when the actual limit is one network device per thread.
> 
> I think you can just kill the dev member of pktgen_net and the two or
> three lines of code that touch it.

Good point!

It is used by pktgen_device_event() to check if the device generates the
event is the one in our namespace.

It is safe to continue the search even if it is not in our namespace,
but it is not efficient. Probably we need to make pktgen_threads list
per-namespace.

Re: [Patch net-next v2] pktgen: support net namespace

[Eric W. Biederman]

Cong Wang <amwang@redhat.com> writes:

> On Mon, 2013-01-28 at 18:36 -0800, Eric W. Biederman wrote:
>> Cong Wang <amwang@redhat.com> writes:
>> 
>> > From: Cong Wang <amwang@redhat.com>
>> >
>> > v2: remove a useless check
>> >
>> > This patch add net namespace to pktgen, so that
>> > we can use pktgen in different namespaces.
>> >
>> > Cc: Eric W. Biederman <ebiederm@xmission.com>
>> > Cc: David S. Miller <davem@davemloft.net>
>> > Signed-off-by: Cong Wang <amwang@redhat.com>
>> >
>> > ---
>> >  net/core/pktgen.c |  123 +++++++++++++++++++++++++++++++++++------------------
>> >  1 files changed, 81 insertions(+), 42 deletions(-)
>> 
>> Skiming through this again I have spotted what looks like a pretty
>> major bug.  You are limiting yourself to one network device per network
>> namespace when the actual limit is one network device per thread.
>> 
>> I think you can just kill the dev member of pktgen_net and the two or
>> three lines of code that touch it.
>
> Good point!
>
> It is used by pktgen_device_event() to check if the device generates the
> event is the one in our namespace.

Which of course is trivial with dev_net()...;

> It is safe to continue the search even if it is not in our namespace,
> but it is not efficient. Probably we need to make pktgen_threads list
> per-namespace.

Having looked at the code a bit more I think the solution really is to
make the proc files per network namespace as you are doing, but to leave
the threads per cpu.  Then it is just a matter of adding for_each_net
loops in the in the paths that add and remove the proc files.

The per interface proc files would of course only show up in a single
network namespace.

It is still a proc file per network namespace and per cpu but that is
better than threads that try and consume cpu resources.  Especially
since the threads when runnign try and consume 100% of the cpu for
transmitting packets.

Eric

Re: [Patch net-next v2] pktgen: support net namespace

[Cong Wang]

On Mon, 2013-01-28 at 19:33 -0800, Eric W. Biederman wrote:
> Cong Wang <amwang@redhat.com> writes:
> 
> > On Mon, 2013-01-28 at 18:36 -0800, Eric W. Biederman wrote:
> >> Cong Wang <amwang@redhat.com> writes:
> >> 
> >> > From: Cong Wang <amwang@redhat.com>
> >> >
> >> > v2: remove a useless check
> >> >
> >> > This patch add net namespace to pktgen, so that
> >> > we can use pktgen in different namespaces.
> >> >
> >> > Cc: Eric W. Biederman <ebiederm@xmission.com>
> >> > Cc: David S. Miller <davem@davemloft.net>
> >> > Signed-off-by: Cong Wang <amwang@redhat.com>
> >> >
> >> > ---
> >> >  net/core/pktgen.c |  123 +++++++++++++++++++++++++++++++++++------------------
> >> >  1 files changed, 81 insertions(+), 42 deletions(-)
> >> 
> >> Skiming through this again I have spotted what looks like a pretty
> >> major bug.  You are limiting yourself to one network device per network
> >> namespace when the actual limit is one network device per thread.
> >> 
> >> I think you can just kill the dev member of pktgen_net and the two or
> >> three lines of code that touch it.
> >
> > Good point!
> >
> > It is used by pktgen_device_event() to check if the device generates the
> > event is the one in our namespace.
> 
> Which of course is trivial with dev_net()...;
> 
> > It is safe to continue the search even if it is not in our namespace,
> > but it is not efficient. Probably we need to make pktgen_threads list
> > per-namespace.
> 
> Having looked at the code a bit more I think the solution really is to
> make the proc files per network namespace as you are doing, but to leave
> the threads per cpu.  Then it is just a matter of adding for_each_net
> loops in the in the paths that add and remove the proc files.

Hmm?

pktgen creates each thread/proc file for each cpu, since proc files are
per-namespace, we will have nr_cpu*nr_ns such proc files and threads.

It hard to improve this due to this kind of design.

I already finished v3 patch which makes pktgen_threads list per-ns, so
far it works well. I am still testing it.

Re: [Patch net-next v2] pktgen: support net namespace

[Eric W. Biederman]

Cong Wang <amwang@redhat.com> writes:

> On Mon, 2013-01-28 at 19:33 -0800, Eric W. Biederman wrote:
>> Cong Wang <amwang@redhat.com> writes:
>> 
>> > On Mon, 2013-01-28 at 18:36 -0800, Eric W. Biederman wrote:
>> >> Cong Wang <amwang@redhat.com> writes:
>> >> 
>> >> > From: Cong Wang <amwang@redhat.com>
>> >> >
>> >> > v2: remove a useless check
>> >> >
>> >> > This patch add net namespace to pktgen, so that
>> >> > we can use pktgen in different namespaces.
>> >> >
>> >> > Cc: Eric W. Biederman <ebiederm@xmission.com>
>> >> > Cc: David S. Miller <davem@davemloft.net>
>> >> > Signed-off-by: Cong Wang <amwang@redhat.com>
>> >> >
>> >> > ---
>> >> >  net/core/pktgen.c |  123 +++++++++++++++++++++++++++++++++++------------------
>> >> >  1 files changed, 81 insertions(+), 42 deletions(-)
>> >> 
>> >> Skiming through this again I have spotted what looks like a pretty
>> >> major bug.  You are limiting yourself to one network device per network
>> >> namespace when the actual limit is one network device per thread.
>> >> 
>> >> I think you can just kill the dev member of pktgen_net and the two or
>> >> three lines of code that touch it.
>> >
>> > Good point!
>> >
>> > It is used by pktgen_device_event() to check if the device generates the
>> > event is the one in our namespace.
>> 
>> Which of course is trivial with dev_net()...;
>> 
>> > It is safe to continue the search even if it is not in our namespace,
>> > but it is not efficient. Probably we need to make pktgen_threads list
>> > per-namespace.
>> 
>> Having looked at the code a bit more I think the solution really is to
>> make the proc files per network namespace as you are doing, but to leave
>> the threads per cpu.  Then it is just a matter of adding for_each_net
>> loops in the in the paths that add and remove the proc files.
>
> Hmm?
>
> pktgen creates each thread/proc file for each cpu, since proc files are
> per-namespace, we will have nr_cpu*nr_ns such proc files and threads.

I was thinking in pktgen_create_thread to do something like:

	for_each_net(net) {
		struct pktgen_net *pn = net_generic(net, pg_net_id);
		pe = proc_create_data(t->tsk->comm, 0600, pn->proc_dir,
				      &pktgen_thread_fops, t);
		if (!pe) {
			....
                }
        }

And in pktgen_thread_write do something like:
	struct pktgen_net *pn = net_generic(current->nsproxy->net_ns, pg_net_id);
	pktgen_add_device(pn, t, f);

Using the seq_net infrastructure you can avoid using current but that
is not necessary to illustrate the basic idea.

> It hard to improve this due to this kind of design.

Whatever works but I don't think it looks that hard.

> I already finished v3 patch which makes pktgen_threads list per-ns, so
> far it works well. I am still testing it.

Fair enough.  Working code is a good place to start.

Eric