Skip to contentSkip to conversation

pktgen: support net namespace

PATCH Superseded Cong Wang 2 participants 6 messages Jan 29, 2013

Subsystems (2)
Networking Network drivers

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Cong Wang

author
#0 lore raw

PATCH v2 1/1 pktgen: support net namespace

From: Cong Wang <[email protected]>

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 <[email protected]>
Cc David S. Miller <[email protected]>
Signed-off-by Cong Wang <[email protected]>
---
 net/core/pktgen.c |  123 +++++++++++++++++++++++++++++++++++------------------
 1 files changed, 81 insertions(+), 42 deletions(-)
net/core/pktgen.c +81 -42
index b29dacf..c217033 100644
@@ -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);
#1 lore raw
Quoted 7 lines · Cong Wang

From: Cong Wang <[email protected]>

v2: remove a useless check

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

Quoted 7 lines
Cc Eric W. Biederman <[email protected]>
Cc David S. Miller <[email protected]>
Signed-off-by Cong Wang <[email protected]>
---
 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.

net/core/pktgen.c struct pktgen_thread { · line 424

Quoted 5 lines
net/core/pktgen.c +5 -0
+static int pg_net_id __read_mostly;
+
+struct pktgen_net {
+	struct net		*net;
+	struct net_device	*dev;

        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

net/core/pktgen.c struct pktgen_thread { · line 426

Quoted 2 lines
net/core/pktgen.c +1 -0
+	struct proc_dir_entry	*proc_dir;
 };

Eric

author
#2 lore raw
Quoted 17 lines · Eric W. Biederman
Quoted 7 lines · Cong Wang

From: Cong Wang <[email protected]>

v2: remove a useless check

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

Quoted 7 lines
Cc Eric W. Biederman <[email protected]>
Cc David S. Miller <[email protected]>
Signed-off-by Cong Wang <[email protected]>
---
 net/core/pktgen.c |  123 +++++++++++++++++++++++++++++++++++------------------
 1 files changed, 81 insertions(+), 42 deletions(-)
Quoted 6 lines

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.

#3 lore raw
Quoted 22 lines · Cong Wang
Quoted 21 lines · Eric W. Biederman
Quoted 7 lines · Cong Wang

From: Cong Wang <[email protected]>

v2: remove a useless check

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

Quoted 7 lines
Cc Eric W. Biederman <[email protected]>
Cc David S. Miller <[email protected]>
Signed-off-by Cong Wang <[email protected]>
---
 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.

Quoted 7 lines
Quoted 2 lines

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()...;

Quoted 3 lines

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

author
#4 lore raw
Quoted 34 lines · Eric W. Biederman
Quoted 22 lines · Cong Wang
Quoted 21 lines · Eric W. Biederman
Quoted 7 lines · Cong Wang

From: Cong Wang <[email protected]>

v2: remove a useless check

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

Quoted 7 lines
Cc Eric W. Biederman <[email protected]>
Cc David S. Miller <[email protected]>
Signed-off-by Cong Wang <[email protected]>
---
 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.

Quoted 7 lines
Quoted 2 lines

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()...;

Quoted 8 lines
Quoted 3 lines

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.

#5 lore raw
Quoted 44 lines · Cong Wang
Quoted 34 lines · Eric W. Biederman
Quoted 22 lines · Cong Wang
Quoted 21 lines · Eric W. Biederman
Quoted 7 lines · Cong Wang

From: Cong Wang <[email protected]>

v2: remove a useless check

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

Quoted 7 lines
Cc Eric W. Biederman <[email protected]>
Cc David S. Miller <[email protected]>
Signed-off-by Cong Wang <[email protected]>
---
 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.

Quoted 7 lines
Quoted 2 lines

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()...;

Quoted 8 lines
Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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

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

Quoted 2 lines

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