Re: [B.A.T.M.A.N.] /proc vis rework
And I wanted to ask, what do you think about unifying the specific help output? For instance having this Usage: ...-header and the alignment for the following items the same way as it is done for other batctl commands as well. I thought about that. However the architecture allows different formats to be easily added. Different formats might need different optional arguments. It just seems easier the way it is at the moment. Andrew
Re: [B.A.T.M.A.N.] [patchv2] batman-adv: Use printk(%pM) for MAC addres
Hello Andrew,
i've checked your patch against older kernel, and there seem to be some
compile problems. E.g. compiling against 2.6.26, i find:
CC [M] /home/dotslash/msrc/batman-svn/batman-adv-kernelland/bat_printk.o
/home/dotslash/msrc/batman-svn/batman-adv-kernelland/bat_printk.c: In function
‘pointer’:
/home/dotslash/msrc/batman-svn/batman-adv-kernelland/bat_printk.c:564: error:
implicit declaration of function ‘dereference_function_descriptor’
/home/dotslash/msrc/batman-svn/batman-adv-kernelland/bat_printk.c:564: warning:
assignment makes pointer from integer without a cast
against 2.6.21, there are a lot more lines of warnings and errors, while
2.6.20 works mostly fine. Could you please review your patch in this regard?
I have nearly all kernels installed so i can give you a list of compile logs if
you
are interested.
Furthermore, i'd suggest to link the bat_printk only if needed to avoid loading
a lot of dead code. Something like this could be done in the Makefile (which i
don't consider the prettiest, but it works ...):
Index: Makefile.kbuild
===
--- Makefile.kbuild (revision 1491)
+++ Makefile.kbuild (working copy)
@@ -32,4 +32,5 @@
endif
obj-m += batman-adv.o
-batman-adv-objs := main.o proc.o send.o routing.o soft-interface.o device.o
translation-table.o bitarray.o hash.o ring_buffer.o vis.o hard-interface.o
aggregation.o originator.o
+batman-adv-objs := main.o proc.o send.o routing.o soft-interface.o device.o
translation-table.o bitarray.o hash.o ring_buffer.o vis.o hard-interface.o
aggregation.o originator.o $(shell [ 2 -eq $(VERSION) ] [ 6 -eq
$(PATCHLEVEL) ] [ $(SUBLEVEL) -le 28 ] echo bat_printk.o)
+
Another approach would be to have a big ifdef in the bat_printk file,
but marek did not like this idea. Any other ideas about this?
best regards
Simon
On Fri, Dec 04, 2009 at 10:42:51PM +0100, Andrew Lunn wrote:
printk() since kernel version 2.6.29 has supported printing MAC
addresses directly, as an extension to the %p processing. This patch
makes use of this for printk() and bat_dbg(). This will remove the
overhead of using addr_to_string() which is normally never actually
output.
For kernels older than 2.6.29 the printk support from 2.6.31 has been
imported into batman-adv and will be used for formatting the output
before passing it onto the native printk() function.
---
Fix the typo found by Gus Wirth.
grep pMs suggests the typo only occurred once.
Signed-off-by: Andrew Lunn and...@lunn.ch
Index: Makefile.kbuild
===
--- Makefile.kbuild (revision 1490)
+++ Makefile.kbuild (working copy)
@@ -32,4 +32,5 @@
endif
obj-m += batman-adv.o
-batman-adv-objs := main.o proc.o send.o routing.o soft-interface.o device.o
translation-table.o bitarray.o hash.o ring_buffer.o vis.o hard-interface.o
aggregation.o originator.o
+batman-adv-objs := main.o proc.o send.o routing.o soft-interface.o device.o
translation-table.o bitarray.o hash.o ring_buffer.o vis.o hard-interface.o
aggregation.o originator.o bat_printk.o
+
Index: translation-table.c
===
--- translation-table.c (revision 1490)
+++ translation-table.c (working copy)
@@ -61,7 +61,6 @@
struct hna_local_entry *hna_local_entry;
struct hna_global_entry *hna_global_entry;
struct hashtable_t *swaphash;
- char hna_str[ETH_STR_LEN];
unsigned long flags;
spin_lock_irqsave(hna_local_hash_lock, flags);
@@ -74,19 +73,17 @@
return;
}
- addr_to_string(hna_str, addr);
-
/* only announce as many hosts as possible in the batman-packet and
space in batman_packet-num_hna That also should give a limit to
MAC-flooding. */
if ((num_hna + 1 (ETH_DATA_LEN - BAT_PACKET_LEN) / ETH_ALEN) ||
(num_hna + 1 255)) {
- bat_dbg(DBG_ROUTES, Can't add new local hna entry (%s): number
of local hna entries exceeds packet size \n, hna_str);
+ bat_dbg(DBG_ROUTES, Can't add new local hna entry (%pM):
number of local hna entries exceeds packet size \n, addr);
return;
}
- bat_dbg(DBG_ROUTES, Creating new local hna entry: %s \n,
- hna_str);
+ bat_dbg(DBG_ROUTES, Creating new local hna entry: %pM \n,
+ addr);
hna_local_entry = kmalloc(sizeof(struct hna_local_entry), GFP_ATOMIC);
if (!hna_local_entry)
@@ -201,12 +198,9 @@
static void hna_local_del(struct hna_local_entry *hna_local_entry,
char *message)
{
- char hna_str[ETH_STR_LEN];
+ bat_dbg(DBG_ROUTES, Deleting local hna entry (%pM): %s \n,
+ hna_local_entry-addr, message);
- addr_to_string(hna_str, hna_local_entry-addr);
- bat_dbg(DBG_ROUTES, Deleting local
Re: [B.A.T.M.A.N.] /proc vis rework
Hmm, everything related to vis-format can be removed from the
kernel(-module) as well, can't it?
This patch applying on top of your batman-adv patch should remove
the vis-format-proc entry and all this dead code if I didn't miss
anything.
Cheers, Linus
diff -ru batman-adv-kernelland/proc.c batman-adv-kernelland2/proc.c
--- batman-adv-kernelland/proc.c 2009-12-13 20:53:50.0 +0100
+++ batman-adv-kernelland2/proc.c 2009-12-13 20:53:07.0 +0100
@@ -29,8 +29,6 @@
#include vis.h
#include compat.h
-static uint8_t vis_format = DOT_DRAW;
-
static struct proc_dir_entry *proc_batman_dir, *proc_interface_file;
static struct proc_dir_entry *proc_orig_interval_file, *proc_originators_file;
static struct proc_dir_entry *proc_transt_local_file;
@@ -403,11 +401,8 @@
struct vis_info_entry *entries;
HLIST_HEAD(vis_if_list);
int i;
- uint8_t current_format;
char tmp_addr_str[ETH_STR_LEN];
- current_format = vis_format;
-
rcu_read_lock();
if (list_empty(if_list) || (!is_vis_server())) {
rcu_read_unlock();
@@ -473,55 +468,6 @@
return single_open(file, proc_vis_read, NULL);
}
-static int proc_vis_format_read(struct seq_file *seq, void *offset)
-{
- uint8_t current_format = vis_format;
-
- seq_printf(seq, [%c] %s\n,
- (current_format == DOT_DRAW) ? 'x' : ' ',
- VIS_FORMAT_DD_NAME);
- seq_printf(seq, [%c] %s\n,
- (current_format == JSON) ? 'x' : ' ',
- VIS_FORMAT_JSON_NAME);
- return 0;
-}
-
-static int proc_vis_format_open(struct inode *inode, struct file *file)
-{
- return single_open(file, proc_vis_format_read, NULL);
-}
-
-static ssize_t proc_vis_format_write(struct file *file,
- const char __user *buffer,
- size_t count, loff_t *ppos)
-{
- char *vis_format_string;
- int not_copied = 0;
-
- vis_format_string = kmalloc(count, GFP_KERNEL);
-
- if (!vis_format_string)
- return -ENOMEM;
-
- not_copied = copy_from_user(vis_format_string, buffer, count);
- vis_format_string[count - not_copied - 1] = 0;
-
- if (strcmp(vis_format_string, VIS_FORMAT_DD_NAME) == 0) {
- printk(KERN_INFO batman-adv:Setting VIS output format to: %s\n,
- VIS_FORMAT_DD_NAME);
- vis_format = DOT_DRAW;
- } else if (strcmp(vis_format_string, VIS_FORMAT_JSON_NAME) == 0) {
- printk(KERN_INFO batman-adv:Setting VIS output format to: %s\n,
- VIS_FORMAT_JSON_NAME);
- vis_format = JSON;
- } else
- printk(KERN_ERR batman-adv:Unknown VIS output format: %s\n,
- vis_format_string);
-
- kfree(vis_format_string);
- return count;
-}
-
static int proc_aggr_read(struct seq_file *seq, void *offset)
{
seq_printf(seq, %i\n, atomic_read(aggregation_enabled));
@@ -585,15 +531,6 @@
.release = single_release,
};
-static const struct file_operations proc_vis_format_fops = {
- .owner = THIS_MODULE,
- .open = proc_vis_format_open,
- .read = seq_read,
- .write = proc_vis_format_write,
- .llseek = seq_lseek,
- .release = single_release,
-};
-
static const struct file_operations proc_vis_fops = {
.owner = THIS_MODULE,
.open = proc_vis_open,
@@ -757,17 +694,6 @@
return -EFAULT;
}
- proc_vis_format_file = create_proc_entry(PROC_FILE_VIS_FORMAT,
- S_IWUSR | S_IRUGO,
- proc_batman_dir);
- if (proc_vis_format_file) {
- proc_vis_format_file-proc_fops = proc_vis_format_fops;
- } else {
- printk(KERN_ERR batman-adv: Registering the '/proc/net/%s/%s' file failed\n, PROC_ROOT_DIR, PROC_FILE_VIS_FORMAT);
- cleanup_procfs();
- return -EFAULT;
- }
-
proc_aggr_file = create_proc_entry(PROC_FILE_AGGR, S_IWUSR | S_IRUGO,
proc_batman_dir);
if (proc_aggr_file) {
diff -ru batman-adv-kernelland/vis.h batman-adv-kernelland2/vis.h
--- batman-adv-kernelland/vis.h 2009-12-13 20:53:26.0 +0100
+++ batman-adv-kernelland2/vis.h 2009-12-13 20:53:07.0 +0100
@@ -45,11 +45,6 @@
uint8_t mac[ETH_ALEN];
};
-enum vis_formats {
- DOT_DRAW,
- JSON,
-};
-
extern struct hashtable_t *vis_hash;
extern spinlock_t vis_hash_lock;
signature.asc
Description: Digital signature
[B.A.T.M.A.N.] wireshark-batman-adv: Displaying issue with vis-packets
The second originator field of a vis client update packet looks a bit odd in wireshark. Sven :)? (have a look at the attached cap-file for an example) I was also wondering about renaming this field maybe, in the beginning I got quite confused by seeing two identical originator fields in most vis packets. Maybe it should be renamed to previous sender or forwarding originator instead? Cheers, Linus batman-adv-vis.cap Description: application/cap signature.asc Description: Digital signature
Re: [B.A.T.M.A.N.] [PATCH] Removing the big batman lock
Hello Andrew,
sorry for the late answer and thank you for your remarks.
I like your idea of the static hash iterator and have got this implemented in
svn revision 1499. This should remove the problems about the memory leaks
with dangling hash iterators. What do you think?
As the patch obviously has some more severe problems (deadlocks etc) i will
have to review this seperately ... :(
On Mon, Nov 30, 2009 at 11:09:02AM +0100, Andrew Lunn wrote:
On Sun, Nov 29, 2009 at 09:09:50PM +0100, Simon Wunderlich wrote:
I did some testing, including loading, unloading, killing individual
nodes etc, which seems to be clean so far. However there might be
more race conditions introduced by this large patch, and i'm therefore
requesting a careful review before committing.
Hi Simon
I've not done a careful review yet, just a quick look.
Two idea for improvements:
In the purge code, add some debug code which looks at the refcount
value. If it is not between 1 and 5, prinkt() a warning what there is
probably a missing refcount operation. Since the purge code does not
run very often, it should not add much overhead, yet it is still a
useful debug tool.
The following bit of code happens quite a lot:
while (NULL != (orig_node = orig_hash_iterate(hashit, orig_node))) {
There is also a comment about having to free hashit, if you don't
iterate to the end of the hash. How about refactoring this, more like
the linux list.h.
Make hashit a stack variable, with an init macro:
#define HASHIT(name) struct hash_it_t name = { .index = -1, .bucket = NULL, \
.prev_bucket=NULL, \
.first_bucket=NULL }
and a macro for iterating over the hash
HASHIT(hashit);
orig_hash_for_each(orig_node, hashit) {
foo(orig_node);
}
Andrew
___
B.A.T.M.A.N mailing list
b.a.t.m@lists.open-mesh.net
https://lists.open-mesh.net/mm/listinfo/b.a.t.m.a.n
signature.asc
Description: Digital signature
