Thanks, Maxim. Assuming that's the last of the additional comments, I think
I've cleaned everything up ready for v3. That is, besides the potential issue
with libatomic.
> > +/**
> > + * Clean up any stale flows within a fraglist
> > + *
> > + * @param fl Pointer to the shared fraglist to clean stale flows from
> > + * @param out The queue to which reassembled packets should be
> > written
> > + * @param force Whether all flows in the fraglist should be
> > considered stale
> > + */
> > +static void garbage_collect_fraglist(union fraglist *fl, odp_queue_t out,
> > + bool force)
> > +{
> > + uint64_t time_now;
> > + struct flts timestamp_now;
> > + struct flts elapsed;
> > + uint64_t elapsed_ns;
> > + union fraglist oldfl;
> > +
> > +redo:;
>
> here it looks like easy to change to while(1)
Good catch. I must have missed this one somehow in refactoring.
> > +int reassemble_ipv4_packets(union fraglist *fraglists, int num_fraglists,
> > + struct packet *fragments, int num_fragments,
> > + odp_queue_t out)
> > +{
> > + int i;
> > + int packets_reassembled = 0;
> > +
> > + for (i = 0; i < num_fragments; ++i) {
> > + struct packet frag;
> > + odph_ipv4hdr_t *hdr;
> > + uint16_t frag_payload_len;
> > + uint16_t frag_reass_payload_len;
> > + int status;
> > +
> > + frag = fragments[i];
> > + hdr = odp_packet_data(frag.handle);
> > + frag_payload_len = ipv4hdr_payload_len_oct(*hdr);
> > + frag_reass_payload_len = ipv4hdr_reass_payload_len_oct(*hdr);
> > +
> > + /*
> > + * Find the appropriate hash map bucket for fragments in this
> > + * flow. In the case of collisions, fragments for multiple flows
> > + * are simply stored in the same list.
> > + */
> > + uint32_t key = hash(hdr);
> > + union fraglist *fl = &fraglists[key % num_fraglists];
> > +
>
> move key and fl inside add_frag_to_fraglist() and arguments might be hdr
> and outq. You call add_frag_to_fraglist() only in single place so it has
> to be ok.
Ehh, I'm not so sure about this one. The three additional parameters that
would need to be passed to add_frag_to_fraglist seems a bit clumsy (hdr,
num_fraglists, fraglists). I don't mind moving those declarations to the
top of the scope though.
> > + /**
> > + * The sum of the payloads of the fragments in this list
> > + *
> > + * That is, the size of reassembling all the fragments in this
> > + * list into one big packet (minus the header).
> > + */
> > + unsigned int part_len:14;
> > +
> > + /**
> > + * The smallest reassembled payload length upper bound from
> > + * all fragments in this list
> > + *
> > + * This is the threshold over which, given the right
> > + * circumstances, "part_len" might indicate that we are able
> > + * to reassemble a packet.
> > + */
> > + unsigned int whole_len:14;
>
> comment might be not for this place exactly, but for all bit fields in
> the code. In api like classification we use uint32_t, i.e. specify size
> exactly and not use native c types like int, long.
Fair enough. I'm not sure whether the type actually matters outside of
signedness, but I'll roll with it anyway (using sized types suitable to the
bit field declarations in the code).
