bcronje at gmail
Oct 26, 2009, 4:57 AM
Post #3 of 3
(751 views)
Permalink
|
Hi Luca,
I've had a quick look at the new hashing code and it looks much better,
thank you. You are absolutely right in regards to fragmented packets. For
applications that rely on per flow state (to do things like TCP re-assembly)
it is absolutely *critical* that all flow packets are send to the same
cluster socket. To that affect I would rather loose fine-grained per-flow
clustering by excluding L4 port information altogether during hashing, or
change the hashing algorithm to take enable_ip_defrag variable into
consideration such as the code below:
inline u_int32_t hash_pkt_header(struct pfring_pkthdr * hdr, u_char
mask_src,
u_char mask_dst)
{
return (hash_pkt(hdr->parsed_pkt.vlan_id,
hdr->parsed_pkt.l3_proto,
mask_src ? 0 : hdr->parsed_pkt.ipv4_src,
mask_dst ? 0 : hdr->parsed_pkt.ipv4_dst,
(mask_src || !enable_ip_defrag) ? 0 :
hdr->parsed_pkt.l4_src_port,
(mask_dst || !enable_ip_defrag) ? 0 :
hdr->parsed_pkt.l4_dst_port));
}
What are your thoughts on this?
Kind regards
Beyers Cronje
On Sat, Oct 24, 2009 at 7:18 PM, Luca Deri <deri [at] ntop> wrote:
> Beyers
> you're right my fault. I have changed the hashing code, please let me know
> what you think. I;m unsure if hashing should include ports as they are not
> present in fragmented packets. What do you think?
>
> Cheers Luca
>
>
> On Oct 19, 2009, at 12:06 AM, Beyers Cronje wrote:
>
> Hi guys,
>>
>> Is per flow clustering currently in a functional state? Looking through
>> the hash_skb() code in pf_ring.c there are a couple of things that seem
>> strange:
>>
>> 1. The first check for per-flow clustering is the following code:
>>
>> if (skb->len > sizeof(struct iphdr) + sizeof(struct tcphdr)) {
>> ........
>> } else
>> idx = skb->len;
>>
>> This seems wrong, as small UDP packets and TCP acknowledgements etc. will
>> be indexed based on the skb length instead of a proper tcp/udp hash. Also
>> there doesnt seem to be a check that the packet is in fact an IP packet.
>>
>> 2. Inside hash_skb() pointers to TCP and UDP headers are calculated with
>> 'sizeof(struct iphdr)', which does not consider IP header options. Surely
>> these should make use of 'ip->ihl << 2' or 'ip->ihl * 4' ?
>>
>>
>> Kind regards
>>
>> Beyers Cronje
>> _______________________________________________
>> Ntop-misc mailing list
>> Ntop-misc [at] listgateway
>> http://listgateway.unipi.it/mailman/listinfo/ntop-misc
>>
>
> _______________________________________________
> Ntop-misc mailing list
> Ntop-misc [at] listgateway
> http://listgateway.unipi.it/mailman/listinfo/ntop-misc
>
|
