[Home]

Summary:DAHLIN-00026: [patch] dahdi_dynamic rx buffering, statistics
Reporter:Pavel Selivanov (biohumanoid)Labels:
Date Opened:2008-07-31 00:28:03Date Closed:2019-05-31 09:47:38
Priority:MajorRegression?No
Status:Closed/CompleteComponents:dahdi_dynamic
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) dahdi_dynamic.c.#2.patch
( 1) dahdi_dynamic.c.patch
( 2) dahdi_dynamic.c.printk.patch
( 3) kernel.h.patch
Description:In current implementation:
1. If we have more than one TDMoE device, we'll have a slip/skip. 2 TDMoE devices can have phase jitter.
2. We have no statistics (which is necessary).
3. ztdynamic can't be a master for zaptel (but why not ?)

****** ADDITIONAL INFORMATION ******

complimentary to
http://bugs.digium.com/view.php?id=13205

I'm sure, It's better to use list with pre-allocated buffers (1 memcpy dahdi_dynamic_receive) instead of fifo I've used (2 memcpy, dahdi_dynamic_receive & __ztdynamic_run).
Or even more, it' better to change dynamic/eth/loc , and use the same idea as skbuff (will have only 1 memcpy on rx, and 1 memcpy on tx{in current version, 2 memcpy on tx})
Comments:By: Pavel Selivanov (biohumanoid) 2008-07-31 00:29:15

The patched attached are for zaptel-1.4.11.
If you feel you want to apply them to dahdi, I'll make them for dahdi.
By: Tzafrir Cohen (tzafrir) 2008-09-15 05:01:11

yes, please do provide a version for DAHDI.

Could you please explain what your patch adds, rather than what is missing?
By: Leif Madsen (lmadsen) 2008-09-15 12:41:02

I've removed the link to the patches off of the bug tracker as it made me sufficiently uncomfortable. Any patches which you would like tested can be uploaded directly to the bug tracker which will be controlled through the licensing system built into mantis.

Thanks!
By: Pavel Selivanov (biohumanoid) 2008-09-24 02:06:38

Here is a patched for dahdi.
dahdi_dynamic.c.patch:
Implemented double buffering on rx.                                          
Implemented procfs statistics.                                              
fixed deadlock in dynamic_destroy called with spinlock.

dahdi_dynamic.c.printk.patch:
printk -> module_printk

Running 2 PC's(synced from RTC) connected by TDMoE will show the problem (slip counter will grow...)
By: Tzafrir Cohen (tzafrir) 2008-09-24 02:24:31

Regarding the printk patch: at first glance I think it can mostly go in. Except the following:

module_printk should use the macro THIS_MODULE:

module_printk(level, fmt, args...) printk(level "%s: " fmt, THIS_MODULE->name, ## args)

Actually, this macro is defined twice already. Maybe move it to kernel.h ?
By: Tzafrir Cohen (tzafrir) 2008-09-24 02:37:58

The length of a proc filesystem page is up to 4096 bytes (and not 1024 as in your comment).

If we assume a dynamic span will take up to 128 bytes and ignore everything else for simple calculations, we get some 32 spans printed in one page. Do you ever get close to that in testing?

If we have a proc page that can print more than that, maybe something is not completely OK, and it should be split to one file per span.
By: Pavel Selivanov (biohumanoid) 2008-09-24 03:05:42

>module_printk(level, fmt, args...) printk(level "%s: " fmt, THIS_MODULE->name, ## args)

>Actually, this macro is defined twice already. Maybe move it to kernel.h ?
Yes, you should.

> 1024 as in your comment
artifact from zaptel-1.4...

tasklet statistics takes <= 150 bytes
per-span statistics takes <= 150 bytes

I think 4096/150 - 1 = 26 dynamic spans is much enougth.
i386 can't handle 26000 of interrupts per second...

I've forgot one more bug fixed:
in dahdi_dynamic_unregister, dynamic_destroy(z) can be called with spinlock..
this finally will call checkmaster with spinlock...
By: Pavel Selivanov (biohumanoid) 2008-09-24 06:45:05

Please, remove ztdynamic.c.patch and dahdi_dynamic.c.patch .
dahdi_dynamic.c.#2.patch is tested during 6 hours,
using the same clock source, using different clocksource, transmitting HDLC data.
By: Pavel Selivanov (biohumanoid) 2008-09-29 03:10:07

None of patches suggested by me is included in trunk.
0013562, 0013542,0013205, 0013206
If they are bad (bad coding style, or something else) - please, let me know.

BTW: release 2.0.0 do not compile with HDLC enabled...
By: Pavel Selivanov (biohumanoid) 2008-11-10 18:38:38.000-0600

I've posted the patches for dahdi, they fixes syncronization problems.
http://bugs.digium.com/view.php?id=13205
http://bugs.digium.com/view.php?id=13206
None of them being tested in trunk, but 2.1 release is coming soon, and it have broken dahdi_dynamic, dumb master handling (first registered SPAN is the master), and so on...

Please, let me know, if they are going to be reviewed or not.
Feel free to ask me if I wrote something not clear.
Thanks.
By: Shaun Ruffell (sruffell) 2008-11-25 15:27:28.000-0600

kfifo isn't available in Linux 2.6.9 and dahdi-linux does still have some customers who use a 2.6.9 kernel.

(and sorry about the delay in looking at the patches....it's not intentional)
By: Pavel Selivanov (biohumanoid) 2011-02-16 03:22:43.000-0600

The problem still exists, do you still support <= 2.6.9 ?

Can I add kfifo.c & kfifo.h from 2.6.10, or it will not be clean by the licence ?
If no - I can patch dynamic to use kfifo with > 2.6.9, not use kfifo with <= 2.6.9 .
Just let me know.
By: Pavel Selivanov (biohumanoid) 2011-02-16 04:21:52.000-0600

I can also reinvent a wheel, and use self-made fifo implementation.
kfifo have changed twice (2.6.35 & 2.6.36)...
By: Shaun Ruffell (sruffell) 2012-04-03 17:14:13.026-0500

Pavel, I'm assigning this to you since I think you were going to update the patches now that support for kernels < 2.6.18 have been dropped.
By: Pavel Selivanov (biohumanoid) 2012-04-04 00:47:45.485-0500

Thank you.

I'll do it in 2 steps:
1. Port current implementation
2. Improve it a bit

By: Pavel Selivanov (biohumanoid) 2013-01-16 06:48:25.074-0600

I've attached a new patch for dahdi_dynamic (DAHDI 2.6.1).
It implements receive fifo, and exports dynamic statistics to procfs.

Why dynamic in DAHDI 2.6.1 is bad ?
1. Can not work in tasklet.
All heavy jobs (echo cancel, rx from master span, so, conference) are done in interrupt context.
2. No statistics.
No way to find the source of slips/skips.
3. Although have some kind of rx fifo (actually in dahdi's conf), it's fixed and have size = 2.
4. Due to using general work queue in dahdi_dynamic_eth, have a huge jitter (up to 10ms).

Patch provided implements:
1. receive fifo
Using kfifo.
2. Procfs statistics
3. sync test
diver will let you know, if dynamic is out of sync with dahdi.
It's a frequent problem.

One more reminder – general workqueue, used in dahdi_dynamic_eth is absolutely bad, even with this patch.

By: Pavel Selivanov (biohumanoid) 2013-01-16 06:51:33.916-0600

Some use-cases:
https://issues.asterisk.org/jira/browse/DAHLIN-25?focusedCommentId=201604&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-201604

By: Shaun Ruffell (sruffell) 2013-01-20 21:14:49.438-0600

Just FYI, I haven't looked closely at the new patch but it didn't compile. At least against 3.2.27 (I haven't tried any other releases).

Also, when I run it though the kernel [checkpatch.pl|http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=scripts/checkpatch.pl] script, I get the following summary:
total: 45 errors, 34 warnings, 425 lines checked

Care to fix that up?
By: Pavel Selivanov (biohumanoid) 2013-01-21 04:10:24.823-0600

{quote}
Just FYI, I haven't looked closely at the new patch but it didn't compile. At least against 3.2.27 (I haven't tried any other releases).

Also, when I run it though the kernel checkpatch.pl script, I get the following summary:
total: 45 errors, 34 warnings, 425 lines checked
{quote}
compile fine on 3.2.9.
Ooops, I'll check with checkpatch.
Please, give me 1 day, I'll clean it up.

By: Shaun Ruffell (sruffell) 2013-01-21 09:54:16.968-0600

Looking into the build failures I saw, it appears that there are changes to include/dahdi/kernel.h that are needed as well?

{noformat}
make -C drivers/dahdi/firmware firmware-loaders
make[1]: Entering directory `/home/sruffell/dahdi-linux/drivers/dahdi/firmware'
make[1]: Leaving directory `/home/sruffell/dahdi-linux/drivers/dahdi/firmware'
make -C /lib/modules/3.2.27/build SUBDIRS=/home/sruffell/dahdi-linux/drivers/dahdi DAHDI_INCLUDE=/home/sruffell/dahdi-linux/include DAHDI_MODULES_EXTRA=" " HOTPLUG_FIRMWARE=yes
make[1]: Entering directory `/home/sruffell/linux-3.2.27'
 CC [M]  /home/sruffell/dahdi-linux/drivers/dahdi/dahdi_dynamic.o
/home/sruffell/dahdi-linux/drivers/dahdi/dahdi_dynamic.c: In function ‘dahdi_dynamic_process_rxfifo’:
/home/sruffell/dahdi-linux/drivers/dahdi/dahdi_dynamic.c:253: error: ‘struct dahdi_dynamic’ has no member named ‘rxfifo_lock’
/home/sruffell/dahdi-linux/drivers/dahdi/dahdi_dynamic.c:253: error: ‘struct dahdi_dynamic’ has no member named ‘rxfifo’
/home/sruffell/dahdi-linux/drivers/dahdi/dahdi_dynamic.c:253: warning: type defaults to ‘int’ in declaration of ‘__tmp’
/home/sruffell/dahdi-linux/drivers/dahdi/dahdi_dynamic.c:253: error: ‘struct dahdi_dynamic’ has no member named ‘rxfifo’
...
{noformat}
By: Pavel Selivanov (biohumanoid) 2013-01-21 13:16:28.187-0600

I've attached kernel.h patch.
By: Pavel Selivanov (biohumanoid) 2013-01-21 14:35:11.179-0600

BTW: you can download a bundle DAHDI 2.6.1 with patches.
http://parabel.ru/d/dahdi_2.6.1%2B2.6.1-parabel_2.6.2.tar.bz2