Bug 1808

Created attachment 1745 [details]
Patch to have FlowMon IP classification done with tags

With this patch, FlowMon only inspects a packet's content then it is sent the first time (IPv4 SendOutgoing trace).

When the packet is forwarded, received at destination or dropped, only the tag is used.

The direct result is:
1) We can easily add new data to the probe, e.g., the number of hops the packet did.
2) Adding IPv6 support is a snap (just add the proper traces to Ipv6l3) and copy-paste the probes in FlowMon)
3) We can kill the non-standard use of IP header's Identification field - HOORAY!

Moreover, since now also fragments can be classified properly (!!!), the stats are finally "right":

  <FlowProbes>
    <FlowProbe index="0">
      <FlowStats  flowId="1" packets="3735" bytes="2149400" delayFromFirstProbeSum="+0.0ns" >
      </FlowStats>
    </FlowProbe>
    <FlowProbe index="1">
      <FlowStats  flowId="1" packets="7466" bytes="2224020" delayFromFirstProbeSum="+199415389258.0ns" >
      </FlowStats>
    </FlowProbe>
    <FlowProbe index="2">
      <FlowStats  flowId="1" packets="3735" bytes="2149400" delayFromFirstProbeSum="+138731526300.0ns" >
      </FlowStats>
    </FlowProbe>
  </FlowProbes>

FlowProbe with index "1" (the forwarder) reports more packets and more bytes. Of course, more packets, more IP headers, more overhead !

(In reply to Tommaso Pecorella from comment #0)
> ... and this isn't exactly a good idea.
> According to RFCs the Identification field must be zero.
> Only fragmented packets should have a non-zero Identification field and it
> should be random.

Not true according to RFC 6864:
- ID field must be unique for packets that may be fragmented
- ID field can be arbitrary for packets that cannot be fragmented

> 
> The changes needed to fix this are:
> 1) Change IPv4 FlowMon components to identify packets according to something
> else (maybe the Packet Uid, or a packet's hash, or a Tag, or all of them).
> 2) Change how the Identification field is handled by IPv4.
> 
> If this isn't fixed, FlowMon can *not* be used with emulated devices (i.e.,
> in mixed emulated/simulated environments).


The below is not quite the same as bug 904; suggest a more informative comment.

+  // ConstCast: see http://www.nsnam.org/bugzilla/﷒0﷓
+  bool found = ConstCast<Packet> (ipPayload)->PeekPacketTag (fTag);

Probably you're right about ID… I should re-read the RFC carefully.
Somewhat I was focused on the IPv6 ID, which *is* zero for non-fragmented packets.

Anyway, the patch is about having FlowMon not using the ID field, which I think it's a good idea (and it's required for IPv6 anyway, so it's a matter of uniformity).

About that comment, I guess I did copy the lines from somewhere. I'll have to check that as well.

T.

The comment was (is) in the original ipv4-flow-probe.cc.

See
http://code.nsnam.org/ns-3-dev/file/6ead574f588a/src/flow-monitor/model/ipv4-flow-probe.cc

I'm up to removing it everywhere.

T.

(In reply to Tommaso Pecorella from comment #4)
> The comment was (is) in the original ipv4-flow-probe.cc.
> 
> See
> http://code.nsnam.org/ns-3-dev/file/6ead574f588a/src/flow-monitor/model/ipv4-
> flow-probe.cc
> 
> I'm up to removing it everywhere.
> 
> T.

In ipv4-flow-probe.cc, it is pertaining to the RemovePacketTag.  I think the comment is fine to leave as is because it does refer to bug 904.

In your patch, you also apply it to PeekPacketTag (besides RemovePacketTag).

+  // ConstCast: see http://www.nsnam.org/bugzilla/﷒0﷓
+  bool found = ConstCast<Packet> (ipPayload)->PeekPacketTag (fTag);

is this cast even needed (PeekPacketTag already operates on const Packets)?  If not, suggest to remove it.

If you can remove this cast and the comment preceding it, I don't have other comments and suggest to merge.

Corrected the PeekPacketTag and pushed.
changeset:   10586:25727ac2504f