Avoid unnecessary data copying inside dvb_dmx_swfilter_204() function
PATCH 1/1 Avoid unnecessary data copying inside dvb_dmx_swfilter_204() function
Hi Mauro Carvalho Chehab.
I'm sorry for sending this twice. I Don't know yet how to disable HTML formatting permanently.
This patch does a significant performance improvement (for the function involved)
by avoiding move of 188 sized packets during 204 to 188 packet conversion. Also this has a robustness fix at discontinuity points of DMA transfer blocks.
I have tested that this code works.
While using "perf top", I noticed that there were two functions that used CPU:
One was dvb_dmx_swfilter_packet and the other was dvb_dmx_swfilter_204.
dvb_dmx_swfilter_204 converts the packets from 204 byte format into 188 format.
It does so by copying the packet before modifications to it. I noticed that it was a very rare case
(usually at most once in 16K data) when the SYNC byte had to be modified from 0xB8 to 0x47.
So my modification does following things:
* Take a copy of the 188 or 204 bytes only if the packet must be modified or backed up.
* Backing up last < 204 bytes: store only data with the 1st byte as the SYNC byte (0xB8 or 0x47).
This is the robustness fix: back up a beginning of a real packet, not
some unknown data block.
During my testing, this loses some bytes occassionally because of some garbage in DMA transferred data:
I measured the number of lost bytes in the inline function that searches the next SYNC byte.
So the algorithm seems to work, usually there aren't many bursts of skipped bytes after the stream has stabilized.
I wanted to know how many bytes are skipped and in which positions in 16K block:
Jun 26 16:20:37 koivu kernel: demux: skipped 49 bytes at position 3379 Jun 26 16:20:37 koivu kernel: demux: skipped 18 bytes at position 9868 Jun 26 16:20:37 koivu kernel: demux: skipped 30 bytes at position 10090 Jun 26 16:20:38 koivu kernel: demux: skipped 14 bytes at position 7208 Jun 26 16:20:38 koivu kernel: demux: skipped 114 bytes at position 7426 Jun 26 16:20:38 koivu kernel: demux: skipped 103 bytes at position 12920 Jun 26 16:20:38 koivu kernel: demux: skipped 72 bytes at position 13431 Jun 26 16:20:38 koivu kernel: demux: skipped 1 bytes at position 13707 Jun 26 16:20:45 koivu kernel: demux: skipped 140 bytes at position 8476 Jun 26 16:20:46 koivu kernel: demux: skipped 70 bytes at position 11396 Jun 26 16:20:46 koivu kernel: demux: skipped 8 bytes at position 11670 Jun 26 16:20:46 koivu kernel: demux: skipped 2 bytes at position 11882
I measured performance improvement with "Perf top". The CPU was probably mostly at 1Ghz and sometimes at 2Ghz,
so the actual number of samples changed randomly, but the relative performance change between
unaltered dvb_dmx_swfilter_packet() and dvb_dmx_swfilter_204() is significant (unrelated rows removed):
Testing was done with H.264 channel recording, and with Mercurial version of v4l-dvb branch.
The patch below is done against v4l-dvb GIT version.
Original performance:
samples pcnt
function DSO
66.00 14.1%
dvb_dmx_swfilter_packet
[dvb_core]
59.00 12.6%
dvb_dmx_swfilter_204
[dvb_core]
11.00 2.4%mantis_i2c_xfer
[mantis_core]
Performance after patching:
samples pcnt
function DSO
81.00 29.0%
dvb_dmx_swfilter_packet
[dvb_core]
14.00 5.0%
mantis_i2c_xfer
[mantis_core]
8.00 2.9%dvb_dmx_swfilter_204
[dvb_core]
So the relative performance has increased by a mangditude for dvb_dmx_swfilter_204().
This patch is a modified version of the original dvb_dmx_swfilter_204(). I can give this patch for the Linux Kernel with license:
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public License
* as published by the Free Software Foundation; either version 2.1
* of the License, or (at your option) any later version.
as the original license in that file.
Patch written by: Marko Ristola <[email protected]>
Regards,
Marko Ristola
(quoted diff) +0 -0
b/drivers/media/dvb/dvb-core/dvb_demux.c
index 977ddba..627d103 100644
drivers/media/dvb/dvb-core/dvb_demux.c +11 -0
@@ -520,49 +520,60 @@ bailout:EXPORT_SYMBOL (dvb_dmx_swfilter); +static inline int find204Sync (const u8 *buf,int pos,size_t count) +{ +while (likely (pos < count)) { +if (likely (buf[pos] ==0x47 || buf[pos] ==0xB8 )) +break ; + pos++; + } + +return pos; +} + void dvb_dmx_swfilter_204(struct dvb_demux *demux, const u8 *buf,
size_t count)
{
int p = 0, i, j;- u8 tmppack[188];
+ const u8 *q;
spin_lock(&demux->lock);
- if (demux->tsbufp) {
+ if (likely(demux->tsbufp)) { /* tsbuf[0] is now 0x47. */
i = demux->tsbufp;
j = 204 - i;
- if (count < j) {
+ if (unlikely(count < j)) {
memcpy(&demux->tsbuf[i], buf, count);
demux->tsbufp += count;
goto bailout;
}
memcpy(&demux->tsbuf[i], buf, j);- if ((demux->tsbuf[0] == 0x47) || (demux->tsbuf[0] == 0xB8)) {
- memcpy(tmppack, demux->tsbuf, 188);
- if (tmppack[0] == 0xB8)
- tmppack[0] = 0x47;
- dvb_dmx_swfilter_packet(demux, tmppack);
- }
+ if (likely(demux->tsbuf[0] == 0x47)) /* double check */
+ dvb_dmx_swfilter_packet(demux, demux->tsbuf);
demux->tsbufp = 0;
p += j;
}- while (p < count) {
- if ((buf[p] == 0x47) || (buf[p] == 0xB8)) {
- if (count - p >= 204) {
- memcpy(tmppack, &buf[p], 188);
- if (tmppack[0] == 0xB8)
- tmppack[0] = 0x47;
- dvb_dmx_swfilter_packet(demux, tmppack);
- p += 204;
- } else {
- i = count - p; - memcpy(demux->tsbuf, &buf[p], i); - demux->tsbufp = i; - goto bailout;
- }
- } else {
- p++;
+ while (likely((p = find204Sync(buf, p, count)) < count)) {
+ if (unlikely(count - p < 204))
+ break;
+
+ q = &buf[p];
+
+ if (unlikely(*q == 0xB8)) {
+ memcpy(demux->tsbuf, q, 188);
+ demux->tsbuf[0] = 0x47;
+ q = demux->tsbuf;
}+ dvb_dmx_swfilter_packet(demux, q);
+ p += 204;
+ }
+
+ i = count - p;
+ if (likely(i)) {
+ memcpy(demux->tsbuf, &buf[p], i);
+ demux->tsbufp = i;
+ if (unlikely(demux->tsbuf[0] == 0xB8))
+ demux->tsbuf[0] = 0x47;
}bailout:
Hi
Here is an improved version of the original patch:
The original patch removed unnecessary copying for 204 sized packets only. This patch improves performance for 188 sized packets too.
Unnecessary copying means: if dvb_dmx_swfilter(_204)() doesn't have to modify
the source packet, the source packet is delivered for dvb_dmx_swfilter_packet()
without copying.
This assumes, that a DMA transfer won't modify the accepted 188/204 sized packet underneath while dvb_dmx_swfilter_packet() processes it. The assumption is already in dvb_dmx_swfilter_packets().
With tasklets the risk for breaking the assumption is low. If there would be
a normal thread instead of a tasklet, copying from the DMA buffer might come too late.
Could someone test this patch who uses the dvb_dmx_swfilter() function (188 sized)?
So _dvb_dmx_swfilter is now common for both 188 and 204 sized packet parsing.
The measure was done during recording of H.264 steram under VDR, using "perf top -d 10"
PerfTop: 62 irqs/sec kernel:80.6% [1000Hz cycles], (all, 1 CPUs)
samples pcnt
function DSO
339.00 18.1%
dvb_dmx_swfilter_packet
[dvb_core]
315.00 16.9%
acpi_pm_read
[kernel.kallsyms]
70.00 3.7%
_ZN2SI5CRC325crc32EPKcij
/usr/sbin/vdr
53.00 2.8%
mantis_i2c_xfer
[mantis_core]
53.00 2.8%
__mktime_internal
/lib64/libc-2.12.so
39.00 2.1%
_ZN14cAudioRepacker6RepackEP17cRingBufferLinearPKhi
/usr/sbin/vdr
33.00 1.8%
delay_tsc
[kernel.kallsyms]
30.00 1.6%
dvb_dmx_memcopy
[dvb_core]
28.00 1.5%
dvb_ringbuffer_write
[dvb_core]
28.00 1.5%_dvb_dmx_swfilter
[dvb_core]
(quoted diff) +0 -0
b/drivers/media/dvb/dvb-core/dvb_demux.c
index 977ddba..2ddaaaa 100644
drivers/media/dvb/dvb-core/dvb_demux.c +0 -0
@@ -478,95 +478,78 @@ void dvb_dmx_swfilter_packets(struct dvb_demux
*demux, const u8 *buf,
EXPORT_SYMBOL(dvb_dmx_swfilter_packets);
-void dvb_dmx_swfilter(struct dvb_demux *demux, const u8 *buf, size_t count) +static inline int findNextSyncByte(const u8 *buf, int pos, size_t count, const int pktsize)
+{
+ while(likely(pos < count)) {
+ if (likely(buf[pos] == 0x47 || (pktsize == 204 && buf[pos] ==0xB8)))
+ break;
+ pos++;
+ }
+
+ return pos;
+}
+
+/* pktsize must be either 204 or 188. If pktsize is 204, 0xB8 must be accepted for SYNC byte too, but then convert it into 0x47.
+ * Designed pktsize so, that compiler would remove 204 related code during inlining. */
+static inline void _dvb_dmx_swfilter(struct dvb_demux *demux, const u8 *buf, size_t count, const int pktsize)
{
int p = 0, i, j;+ const u8 *q;
spin_lock(&demux->lock);
- if (demux->tsbufp) {
+ if (likely(demux->tsbufp)) { /* tsbuf[0] is now 0x47. */
i = demux->tsbufp;
- j = 188 - i;
- if (count < j) {
+ j = pktsize - i;
+ if (unlikely(count < j)) {
memcpy(&demux->tsbuf[i], buf, count);
demux->tsbufp += count;
goto bailout;
}
memcpy(&demux->tsbuf[i], buf, j);
- if (demux->tsbuf[0] == 0x47)
+ if (likely(demux->tsbuf[0] == 0x47)) /* double check */
dvb_dmx_swfilter_packet(demux, demux->tsbuf);
demux->tsbufp = 0;
p += j;
}- while (p < count) {
- if (buf[p] == 0x47) {
- if (count - p >= 188) {
- dvb_dmx_swfilter_packet(demux, &buf[p]);
- p += 188;
- } else {
- i = count - p; - memcpy(demux->tsbuf, &buf[p], i); - demux->tsbufp = i; - goto bailout;
- }
- } else
- p++;
+ while (likely((p = findNextSyncByte(buf, p, count, pktsize)) <
count)) {
+ if (unlikely(count - p < pktsize))
+ break;
+
+ q = &buf[p];
+
+ if (unlikely(pktsize == 204 && (*q == 0xB8))) {
+ memcpy(demux->tsbuf, q, 188); + demux->tsbuf[0] = 0x47; + q = demux->tsbuf; + } + dvb_dmx_swfilter_packet(demux, q); + p += pktsize;
+ }
+
+ i = count - p;
+ if (likely(i)) {
+ memcpy(demux->tsbuf, &buf[p], i);
+ demux->tsbufp = i;
+ if (unlikely(pktsize == 204 && demux->tsbuf[0] == 0xB8))
+ demux->tsbuf[0] = 0x47;
}bailout:
spin_unlock(&demux->lock);
}
+void dvb_dmx_swfilter(struct dvb_demux *demux, const u8 *buf, size_t count) +{
+ _dvb_dmx_swfilter(demux, buf, count, 188);
+}
+
EXPORT_SYMBOL(dvb_dmx_swfilter);
void dvb_dmx_swfilter_204(struct dvb_demux *demux, const u8 *buf,size_t count)
{
- int p = 0, i, j;
- u8 tmppack[188];
-
- spin_lock(&demux->lock);
-
- if (demux->tsbufp) {
- i = demux->tsbufp;
- j = 204 - i;
- if (count < j) {
- memcpy(&demux->tsbuf[i], buf, count);
- demux->tsbufp += count;
- goto bailout;
- }
- memcpy(&demux->tsbuf[i], buf, j);
- if ((demux->tsbuf[0] == 0x47) || (demux->tsbuf[0] == 0xB8)) {- memcpy(tmppack, demux->tsbuf, 188);
- if (tmppack[0] == 0xB8)
- tmppack[0] = 0x47;
- dvb_dmx_swfilter_packet(demux, tmppack);
- } - demux->tsbufp = 0; - p += j;
- }
-
- while (p < count) {
- if ((buf[p] == 0x47) || (buf[p] == 0xB8)) {
- if (count - p >= 204) {
- memcpy(tmppack, &buf[p], 188);
- if (tmppack[0] == 0xB8)
- tmppack[0] = 0x47;
- dvb_dmx_swfilter_packet(demux, tmppack);
- p += 204;
- } else {
- i = count - p; - memcpy(demux->tsbuf, &buf[p], i); - demux->tsbufp = i; - goto bailout;
- }
- } else {
- p++;
- }
- }
-
-bailout:
- spin_unlock(&demux->lock);
+ _dvb_dmx_swfilter(demux, buf, count, 204);
}
EXPORT_SYMBOL(dvb_dmx_swfilter_204);
Hi
dvb_dmx_swfilter(_204) performance improvement, with packet recovery.
Remove unnecessary copying of 188 and 204 sized packets within dvb_dmx_swfilter() and dvb_dmx_swfilter_204() functions. Recover one packet by backtracking, when there is a short packet with a correct SYNC byte in the middle.
------------------------------------------------------------
This patch tries to recover with backtracking one 188 / 204 sized packet if some garbage is found.
The backtracking recovery case, DVB data:
Packet Num, Packet data
1 0x47 + 99 bytes garbage 2 0x47 + 187 bytes data 3 0x47 + 187 bytes data
Recovery algorithm:
First packet 1's 0x47 is found. Process it, advance 188 bytes. Find next SYNC byte, packet 3 found.
Check packet 3 position - 188: is there a packet? yes, return packet 2.
Advance 188 bytes, return packet 3.
Jun 27 20:44:59 koivu kernel: demux: backtracked 137 bytes into position 442 Jun 27 20:44:59 koivu kernel: demux: backtracked 28 bytes into position 2250 Jun 27 20:45:02 koivu kernel: demux: backtracked 35 bytes into position 2634 Jun 27 20:45:02 koivu kernel: demux: backtracked 12 bytes into position 1398 Jun 27 20:45:03 koivu kernel: demux: skipped 146 bytes at position 378: 09 93 ... Jun 27 20:45:03 koivu kernel: demux: backtracked 177 bytes into position 551 Jun 27 20:45:03 koivu kernel: demux: skipped 14 bytes at position 959: 6c 1e ... Jun 27 20:45:03 koivu kernel: demux: backtracked 191 bytes into position 986
Regards,
Marko Ristola
(quoted diff) +0 -0
b/drivers/media/dvb/dvb-core/dvb_demux.c
index 977ddba..b71d77d 100644
drivers/media/dvb/dvb-core/dvb_demux.c +0 -0
@@ -478,97 +478,96 @@ void dvb_dmx_swfilter_packets(struct dvb_demux
*demux, const u8 *buf,
EXPORT_SYMBOL(dvb_dmx_swfilter_packets);
-void dvb_dmx_swfilter(struct dvb_demux *demux, const u8 *buf, size_t count) +static inline int findNextPacket(const u8 *buf, int pos, size_t count,
+ const int pktsize)
{
- int p = 0, i, j;
+ int start = pos, lost;
- spin_lock(&demux->lock);
-
- if (demux->tsbufp) {
- i = demux->tsbufp;
- j = 188 - i;
- if (count < j) {
- memcpy(&demux->tsbuf[i], buf, count);
- demux->tsbufp += count;
- goto bailout;
- }
- memcpy(&demux->tsbuf[i], buf, j);
- if (demux->tsbuf[0] == 0x47)- dvb_dmx_swfilter_packet(demux, demux->tsbuf);
- demux->tsbufp = 0;
- p += j;
+ while(likely(pos < count)) {
+ if (likely(buf[pos] == 0x47 ||
+ (pktsize == 204 && buf[pos] == 0xB8)))
+ break;
+ pos++;
}- while (p < count) {
- if (buf[p] == 0x47) {
- if (count - p >= 188) {
- dvb_dmx_swfilter_packet(demux, &buf[p]);
- p += 188;
- } else {
- i = count - p; - memcpy(demux->tsbuf, &buf[p], i); - demux->tsbufp = i; - goto bailout;
- }
- } else
- p++;
+ if (unlikely(lost = pos - start)) {
+ /* This garbage is part of a valid packet? */ + int backtrack = pos - pktsize; + if (backtrack >= 0 && (buf[backtrack] == 0x47 ||
+ (pktsize == 204 && buf[backtrack] == 0xB8))) {
+ /* printk("demux: backtracked %d bytes"
+ * "\n", start - backtrack); */
+ return backtrack;
+ }
+ /*printk("demux: skipped %d bytes at %d\n", lost, start); */
}
-bailout:
- spin_unlock(&demux->lock);
+ return pos;
}
-EXPORT_SYMBOL(dvb_dmx_swfilter);
-
-void dvb_dmx_swfilter_204(struct dvb_demux *demux, const u8 *buf, size_t count)
+/* Filter all pktsize= 188 or 204 sized packets and skip garbage. */ +static inline void _dvb_dmx_swfilter(struct dvb_demux *demux, const u8 *buf,
+ size_t count, const int pktsize)
{
int p = 0, i, j;- u8 tmppack[188];
+ const u8 *q;
spin_lock(&demux->lock);
- if (demux->tsbufp) {
+ if (likely(demux->tsbufp)) { /* tsbuf[0] is now 0x47. */
i = demux->tsbufp;
- j = 204 - i;
- if (count < j) {
+ j = pktsize - i;
+ if (unlikely(count < j)) {
memcpy(&demux->tsbuf[i], buf, count);
demux->tsbufp += count;
goto bailout;
}
memcpy(&demux->tsbuf[i], buf, j);- if ((demux->tsbuf[0] == 0x47) || (demux->tsbuf[0] == 0xB8)) {
- memcpy(tmppack, demux->tsbuf, 188);
- if (tmppack[0] == 0xB8)
- tmppack[0] = 0x47;
- dvb_dmx_swfilter_packet(demux, tmppack);
- }
+ if (likely(demux->tsbuf[0] == 0x47)) /* double check */
+ dvb_dmx_swfilter_packet(demux, demux->tsbuf);
demux->tsbufp = 0;
p += j;
}- while (p < count) {
- if ((buf[p] == 0x47) || (buf[p] == 0xB8)) {
- if (count - p >= 204) {
- memcpy(tmppack, &buf[p], 188);
- if (tmppack[0] == 0xB8)
- tmppack[0] = 0x47;
- dvb_dmx_swfilter_packet(demux, tmppack);
- p += 204;
- } else {
- i = count - p; - memcpy(demux->tsbuf, &buf[p], i); - demux->tsbufp = i; - goto bailout;
- }
- } else {
- p++;
+ while (likely((p = findNextPacket(buf, p, count, pktsize)) < count)) {
+ if (unlikely(count - p < pktsize))
+ break;
+
+ q = &buf[p];
+
+ if (unlikely(pktsize == 204 && (*q == 0xB8))) {
+ memcpy(demux->tsbuf, q, 188);
+ demux->tsbuf[0] = 0x47;
+ q = demux->tsbuf;
}+ dvb_dmx_swfilter_packet(demux, q);
+ p += pktsize;
+ }
+
+ i = count - p;
+ if (likely(i)) {
+ memcpy(demux->tsbuf, &buf[p], i);
+ demux->tsbufp = i;
+ if (unlikely(pktsize == 204 && demux->tsbuf[0] == 0xB8))
+ demux->tsbuf[0] = 0x47;
}bailout:
spin_unlock(&demux->lock);
}
+void dvb_dmx_swfilter(struct dvb_demux *demux, const u8 *buf, size_t count) +{
+ _dvb_dmx_swfilter(demux, buf, count, 188);
+}
+
+EXPORT_SYMBOL(dvb_dmx_swfilter);
+
+void dvb_dmx_swfilter_204(struct dvb_demux *demux, const u8 *buf, size_t count)
+{
+ _dvb_dmx_swfilter(demux, buf, count, 204);
+}
+
EXPORT_SYMBOL(dvb_dmx_swfilter_204);
static struct dvb_demux_filter *dvb_dmx_filter_alloc(struct dvb_demux
*demux)