* Re: [PATCH 14/25] Basic implementation for ccid-4 dropped packet
2007-11-01 0:31 [PATCH 14/25] Basic implementation for ccid-4 dropped packet option Leandro
@ 2007-11-01 11:05 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 2+ messages in thread
From: Arnaldo Carvalho de Melo @ 2007-11-01 11:05 UTC (permalink / raw)
To: dccp
Em Wed, Oct 31, 2007 at 09:31:40PM -0300, Leandro escreveu:
> [CCID-4] Basic implementation for ccid-4 dropped packet option as per ccid-4 draft
>
> Signed-off-by: Leandro Melo de Sales <leandro@embedded.ufcg.edu.br>
>
> Index: leandro.new/net/dccp/ccids/lib/tfrc_ccids.h
> =================================> --- leandro.new.orig/net/dccp/ccids/lib/tfrc_ccids.h
> +++ leandro.new/net/dccp/ccids/lib/tfrc_ccids.h
> @@ -36,12 +36,15 @@ enum tfrc_options {
> TFRC_OPT_LOSS_EVENT_RATE = 192,
> TFRC_OPT_LOSS_INTERVALS = 193,
> TFRC_OPT_RECEIVE_RATE = 194,
> + TFRC_OPT_DROPPED_PACKETS = 195, /* as per ccid-4 draft, section 8 */
> };
>
> struct tfrc_options_received {
> u64 tfrcor_seqno:48,
> - tfrcor_loss_intervals_idx:16;
> - u16 tfrcor_loss_intervals_len;
> + tfrcor_loss_intervals_idx:16,
> + tfrcor_dropped_packets_idx:16;
Just make tfrcor_dropped_packets_idx a plain u16.
tfrcor_loss_intervals_idx is part of a bitfield together with
tfrcor_seqno because it is 48 bits and we want to use what is left in a
u64.
Making tfrcor_dropped_packets_idx a 16 bits entry in a 16 bits bitfield
doesn't helps us here and likely produces bad code.
> + u16 tfrcor_loss_intervals_len,
> + tfrcor_dropped_packets_len;
> u32 tfrcor_loss_event_rate;
> u32 tfrcor_receive_rate;
> };
And even leads to some confusion, one would think that the second,
single entry bitfield would use 64 bits, as it is declared u64, but the
rule is:
[acme@doppio ~]$ pahole bitfield
struct tfrc_options_received {
u64 tfrcor_seqno:48; /* 0 8 */
u64 tfrcor_loss_intervals_idx:16; /* 0 8 */
u64 tfrcor_dropped_packets_idx:16; /* 8 8 */
/* Bitfield WARNING: DWARF size=8, real size=2 */
u16 tfrcor_loss_intervals_len; /* 10 2 */
u16 tfrcor_dropped_packets_len; /* 12 2 */
/* XXX 2 bytes hole, try to pack */
u32 tfrcor_loss_event_rate; /* 16 4 */
u32 tfrcor_receive_rate; /* 20 4 */
/* size: 24, cachelines: 1 */
/* sum members: 22, holes: 1, sum holes: 2 */
/* last cacheline: 24 bytes */
}; /* definitions: 1 */
[acme@doppio ~]$
The rule is so confusing that even gcc gets lost and emits debugging
information where it says the type of the bitfield is u64, thus 8 bytes
wide, but the offset of the next field is 2 bytes after the bitfield :-)
So just make it u16 and we'll get this:
[acme@doppio ~]$ pahole bitfield
[acme@doppio ~]$ pahole bitfield
struct tfrc_options_received {
u64 tfrcor_seqno:48; /* 0 8 */
u64 tfrcor_loss_intervals_idx:16; /* 0 8 */
u16 tfrcor_dropped_packets_idx; /* 8 2 */
u16 tfrcor_loss_intervals_len; /* 10 2 */
u16 tfrcor_dropped_packets_len; /* 12 2 */
/* XXX 2 bytes hole, try to pack */
u32 tfrcor_loss_event_rate; /* 16 4 */
u32 tfrcor_receive_rate; /* 20 4 */
/* size: 24, cachelines: 1 */
/* sum members: 22, holes: 1, sum holes: 2 */
/* last cacheline: 24 bytes */
}; /* definitions: 1 */
[acme@doppio ~]$
And now we have a 2 bytes hole, so we may as well just stop
making the first two fields a bitfield.
- Arnaldo
^ permalink raw reply [flat|nested] 2+ messages in thread