Pending dvb_dmx_swfilter(_204)() patch tested enough

Pending dvb_dmx_swfilter(_204)() patch tested enough

[Marko Ristola]

Hi Mauro.

Following patch has been tested enough since last Summer 2010:

"Avoid unnecessary data copying inside dvb_dmx_swfilter_204() function"
https://patchwork.kernel.org/patch/118147/
It modifies both dvb_dmx_swfilter_204() and dvb_dmx_swfilter()  functions.

I Myself tested dvb_dmx_swfilter_204() with terrestrial DVB-C, using 204 sized packets.
Some other Mantis users have satellite receiver equipment, using 188 sized packets, thus
using dvb_dmx_swfilter().

The patch has been in yaVDR/testing branch and used there.

So both functions are tested enough during one half year period.
I was asked by a person this week that if the patch could go forward.

Regards,
Marko Ristola

Re: Pending dvb_dmx_swfilter(_204)() patch tested enough

[Andreas Oberritter]

Hello Marko,

On 03/26/2011 09:20 PM, Marko Ristola wrote:
> Following patch has been tested enough since last Summer 2010:
>
> "Avoid unnecessary data copying inside dvb_dmx_swfilter_204() function"
> https://patchwork.kernel.org/patch/118147/
> It modifies both dvb_dmx_swfilter_204() and dvb_dmx_swfilter()  functions.

sorry, I didn't know about your patch. Can you please resubmit it with
the following changes?

- Don't use camelCase (findNextPacket)

- Remove disabled printk() calls.

- Only one statement per line.
	if (unlikely(lost = pos - start)) {
	while (likely((p = findNextPacket(buf, p, count, pktsize)) < count)) {

- Add white space between while and the opening brace.
	while(likely(pos < count)) {

- Use unsigned data types for pos and pktsize:
	static inline int findNextPacket(const u8 *buf, int pos, size_t count,
	const int pktsize)

The CodingStyle[1] document can serve as a guideline on how to properly
format kernel code.

Does the excessive use of likely() and unlikely() really improve the
performance or is it just a guess?

Regards,
Andreas

[1]
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/CodingStyle

Re: Pending dvb_dmx_swfilter(_204)() patch tested enough

[Mauro Carvalho Chehab]

Em 28-03-2011 09:07, Andreas Oberritter escreveu:
> Hello Marko,
> 
> On 03/26/2011 09:20 PM, Marko Ristola wrote:
>> Following patch has been tested enough since last Summer 2010:
>>
>> "Avoid unnecessary data copying inside dvb_dmx_swfilter_204() function"
>> https://patchwork.kernel.org/patch/118147/
>> It modifies both dvb_dmx_swfilter_204() and dvb_dmx_swfilter()  functions.
> 
> sorry, I didn't know about your patch. Can you please resubmit it with
> the following changes?
> 
> - Don't use camelCase (findNextPacket)
> 
> - Remove disabled printk() calls.
> 
> - Only one statement per line.
> 	if (unlikely(lost = pos - start)) {
> 	while (likely((p = findNextPacket(buf, p, count, pktsize)) < count)) {
> 
> - Add white space between while and the opening brace.
> 	while(likely(pos < count)) {
> 
> - Use unsigned data types for pos and pktsize:
> 	static inline int findNextPacket(const u8 *buf, int pos, size_t count,
> 	const int pktsize)
> 
> The CodingStyle[1] document can serve as a guideline on how to properly
> format kernel code.

A good way for testing coding style is to run the scripts/checkpatch.pl.

It points most of the stuff at CodingStyle.

> Does the excessive use of likely() and unlikely() really improve the
> performance or is it just a guess?

I never tried to perf likely/unlikely, but, AFAIK, it will affect cache miss
rate and will also affect performance on superscalar architecture, as a
branch operation may clean the pipelines. So, avoiding an unneeded branch
will improve speed.

So, it is recommended to use it when you know what you're doing and need to
optimize performance.

There's an interesting explanation about it at:
	http://kerneltrap.org/node/4705

> 
> Regards,
> Andreas
> 
> [1]
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/CodingStyle

Re: Pending dvb_dmx_swfilter(_204)() patch tested enough

[Marko Ristola]

Hello Andreas and Mauro

Thank you for looking at the patch.
Than you Mauro for giving the likely() unlikely() GCC assembly example
and remind of scripts/checkpatch.pl.

28.03.2011 15:07, Andreas Oberritter wrote:
> Hello Marko,
>
> On 03/26/2011 09:20 PM, Marko Ristola wrote:
>> Following patch has been tested enough since last Summer 2010:
>>
>> "Avoid unnecessary data copying inside dvb_dmx_swfilter_204() function"
>> https://patchwork.kernel.org/patch/118147/
>> It modifies both dvb_dmx_swfilter_204() and dvb_dmx_swfilter()  functions.
>
> sorry, I didn't know about your patch. Can you please resubmit it with
> the following changes?
>
> - Don't use camelCase (findNextPacket)
I'll rename it as find_next_packet().

>
> - Remove disabled printk() calls.
I'll do that.

>
> - Only one statement per line.
> 	if (unlikely(lost = pos - start)) {
> 	while (likely((p = findNextPacket(buf, p, count, pktsize))<  count)) {

I'll alter while() line as:

	while (1) {
		p = find_next_packet(buf, p, count, pktsize);
		if (p >= count)
			break;
		if (count - p < pktsize)
			break;



>
> - Add white space between while and the opening brace.
> 	while(likely(pos<  count)) {
Thanks.

>
> - Use unsigned data types for pos and pktsize:
> 	static inline int findNextPacket(const u8 *buf, int pos, size_t count,
> 	const int pktsize)

I'll change them.

>
> The CodingStyle[1] document can serve as a guideline on how to properly
> format kernel code.
Thanks. I have read it, but reading again is always good for learning.
>
> Does the excessive use of likely() and unlikely() really improve the
> performance or is it just a guess?

I'll try to measure performance difference next weekend: I haven't tested the effect
on likely/unlikely operations. I have tested the effect of the whole patch only.
I selected likely and unlikely() sentences very carefully, so they should be correct.



I'm not sure if recovering a packet by backtracking is worth it on my patch:
If recovered packets are typically discarded by dvb_dmx_swfilter_packet() later, I'll drop the code.

Here is a description of the problem as I saw it last Summer:

My DVB card seemed to lose a few packets somewhere in the DMA transfer buffer and then had a short packet
(less than 204 garbage bytes) and then normal good packets.

Backtracking use case (deliver 16K bytes of data for dvb_dmx_swfilter_204() per call):
1. DVB card loses a few packets.
2. DVB card delivers less than 204 bytes garbage, containing packet start byte in the middle.
3. dvb_dmx_swfilter_204() finds start byte from garbage. Deliver 204 sized garbage packet into dvb_dmx_swfilter_packet().
4. dvb_dmx_swfilter_204() detects that packet at (3) was garbage. One good packet can be restored by buffer backtracking.
5. dvb_dmx_swfilter_204() delivers restored packet into dvb_dmx_swfilter_packet().
6. dvb_dmx_swfilter_204() continues to deliver 204 sized packets into dvb_dmx_swfilter_packet().

I suspect that on (5), the restored packet is discarded by dvb_dmx_swfilter_packet() because of
packet counter sequencing error. There is no benefit with recovering the packet in this (typical) case.
dvb_dmx_swfilter_packet() will discard packets until it finds a next group of packets with group
start identifier. Is this correct?

Regards,
Marko Ristola

>
> Regards,
> Andreas
>
> [1]
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/CodingStyle
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>