Any further thoughts on these issues, Maxim? I'm keen to get this patch
merged already!
> > > I raised question about coding style question on today’s arch call
> > > discussion. And agreement was:
> > >
> > > 1. variables are on top. (actually we discussed that but looks like
> > > forget to document.) Some exceptions acceptable if you link to 3-rd
> > > party code which you can not modify. Like netmap.
> >
> > My interpretation was the variable declarations at the top of the
> > function is the default. If you want/need to deviate from the default,
> > you need to document why. One such reason is this reassembly example
> > where an inner block with local variables is simpler (as in less
> > complexity) than calling a separate function. The reason is that the
> > code has multiple outputs which is complicated to handle in C. Inner
> > blocks with local variable declarations is not some obscure C language
> > feature and solves the problem nicely.
>
> I'm glad to hear that there was some discussion around these issues. I do
> think that I have sufficient reason to place variable declarations within
> the body of functions in a few places here, particularly in long functions
> like "main" and "add_fraglist_to_fraglist".
>
> Though these form of declarations are mostly within new scopes, there are
> currently a couple of places in which the declarations are needed for the
> remainder of the function body, and so don't live within new scopes to
> prevent unnecessary indentation. Would this be considered to break this
> newly formed formatting rule?
>
> > > 2. Empty braces {} are ok along intend is clear.
>
> Ok, great! As far as I'm concerned, intent is nearly always clear with these
> sorts of constructs though. If variables are declared at the top of the new
> scope, then it's clear that these variables need only live for the lifetime
> of the scope. What do you consider to be clear intent?
>
> > > gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.3)
> > > odp_ipfragreass-odp_ipfragreass_reassemble.o: In function
> > > `atomic_strong_cas_16':
> > > /opt/Linaro/odp3.git/example/ipfragreass/odp_ipfragreass_helpers.h:123:
> > > undefined reference to `__atomic_compare_exchange_16'
> > > /opt/Linaro/odp3.git/example/ipfragreass/odp_ipfragreass_helpers.h:123:
> > > undefined reference to `__atomic_compare_exchange_16'
> > > /opt/Linaro/odp3.git/example/ipfragreass/odp_ipfragreass_helpers.h:123:
> > > undefined reference to `__atomic_compare_exchange_16'
> >
> > You may need to link with -latomic on certain targets to get 128-bit
> > atomics support.
>
> Also, are you sure you're using v2 of the patch? There was an issue that
> could cause errors of this sort in v1, but as __atomic_compare_exchange_n is
> actually used internally to ODP as well as in this example, it should no
> longer cause a problem if the rest of ODP is able to compile correctly.
>
> > >>>> +#include <stdio.h>
> > >>>> +#include <assert.h>
> > >>>> +
> > >>>> +#include "odp_ipfragreass_ip.h"
> > >>>> +#include "odp_ipfragreass_fragment.h"
> > >>>> +
> > >>>> +int fragment_ipv4_packet(odp_packet_t orig_packet, odp_packet_t *out,
> > >>>> + int *out_len)
> > >>>> +{
> > >>>> + uint32_t orig_len = odp_packet_len(orig_packet);
> > >>>> + odph_ipv4hdr_t *orig_header = odp_packet_data(orig_packet);
> > >>>> + uint32_t header_len = ipv4hdr_ihl(*orig_header);
> > >>>> + uint32_t bytes_remaining = orig_len - header_len;
> > >>>> +
> > >>>> + if (bytes_remaining <= MTU)
> > >>>> + return -1;
> > >>>> +
> > >>>> + /*
> > >>>> + * The main fragmentation loop (continue until all bytes from
> > >>>> the
> > >>>> + * original payload have been assigned to a fragment)
> > >>>> + */
> > >>>> + odp_packet_t frag = orig_packet;
> > >>>
> > >>> 1. why to you need second variable for the same packet handle?
> > >>
> > >> For readability! You're right that I could call the "orig_packet"
> > >> parameter
> > >> "frag" and be done with it, but it's really much clearer if rather than
> > >> reusing the same variable for two effectively unrelated purposes, I use
> > >> the
> > >> wonderful ability granted to us by the programming gods to associate
> > >> values
> > >> with names that describe their purpose. And best of all, for all but the
> > >> most
> > >> primitive or idiotic of compilers, it's completely free! (And, actually,
> > >> probably makes the optimiser's life easier.)
> > >>
> > >
> > > I think it decrease portability.
> > ???
> > I would really like to hear the arguments for this opinion.
> >
> > > I think it decrease portability. It's very hard to argue because
> > > different people look from different angle. And what is nice for one
> > > looks bad for other..
>
> The driving force behind my decision to keep things the way they were written
> here is not from the perspective of aesthetics. Far from it, I've tried to
> abide to the checkpatch script to the letter. This is about code readability.
> Using the name "frag" for the input packet to the function, which is itself
> not expected to be a fragment, is misleading and makes the code harder to
> understand. I'm not willing to compromise on this point. Especially not for
> unsubstantiated claims of increased portability.
>
> > >>> 2. Variable should be declared at the begining of the scope.
> > >>
> > >> Is this a hard requirement? I noticed that it wasn't picked up by
> > >> checkpatch,
> > >> and I can't see it outlined in the kernel's coding style guidelines
> > >> either. I
> > >> don't think this would cause a total readability disaster here, but I do
> > >> think it would make the code at least a little harder to read. Mentally,
> > >> for
> > >> me, the variable declarations are clearly split between auxiliary data
> > >> required for the whole function (declared at the top), and data that is
> > >> instrumental in operating the big ol' while loop (declared just before
> > >> the
> > >> loop).
> > >>
> > >> Admittedly, the function more or less /is/ the loop here though, and
> > >> perhaps
> > >> you could argue that this function is too long as per the kernel style
> > >> guidelines anyway. I use a similar style in other places in the code,
> > >> though,
> > >> where I think it's hugely more useful. For instance, in the function
> > >> "add_fraglist_to_fraglist". It's a big function, so is helped
> > >> significantly
> > >> by this. You could definitely argue that it's too long, but it sort of
> > >> needs
> > >> to be. (In some part, at least, because in my testing the optimiser
> > >> failed to
> > >> perform tail call optimisation.)
> > >>
> > >
> > > this function also can be split.
> >
> > Every function can be slit into smaller functions until there is only
> > one C statement left in each function. This doesn't make it a good
> > idea though.
>
> In this case, I've tried to split things when it's clearly beneficial to do
> so, but have mostly left the cases where it's less clear as single functions
> (after all, it's often easier to factor things out than it is to bring them
> back to a natural integrated state). In some cases (with the caveat of the
> goto, see discussion below), I could split some of these longer functions
> into shorter functions with a number of output parameters, but I'm not sure
> that's necessarily better. (In fact, in "add_fraglist_to_fraglist" I think it
> would often make the code worse, as it may obfuscate some of the core control
> code that makes the algorithm tick.)
>
> > > And goto for hole function is ugly,
> > > which you used to save some space to avoid function splitting.
> > >
> > > things line that "redo:;"
> > >
> > > it's it nicer to do in kernel style?
> > >
> > > static incline int __f(...args...)
> > > {
> > >
> > > }
> > >
> > > void f(..args..)
> > > {
> > > while (!__f(..args.._)) {};
> > >
> > > }
>
> I don't think using a goto for the whole function with only a few pieces of
> state carrying over is /too/ bad, as it effectively mimics tail recursion
> (which was originally used in the code before I found that the optimiser
> wasn't playing nicely with it).
>
> Doing it through an iteration construct over a separate inline function isn't
> a terrible idea though, and is probably workable (if a little awkward from
> the perspective of requiring a "shell" iteration function and an extra out
> parameter for the 'real' return value). It does get rid of the goto and
> potentially allow some of the code to be factored out of the function though
> (see caveat about potentially requiring lots of out parameters), so that's a
> plus. I don't feel that strongly about this, so either way is good.
>
> > >
> > >>> 3. For easy reading it's better to have _pkt postfix or prefix to to
> > >>> handles. Like pkt_orig or orig_pkt. Like we do in implementation.
> > >>> That is optional but it's very easy to see if it's variable or handle.
> > >>
> > >> Hmm... isn't this what we have types for? I'm not a fan of hungarian
> > >> notation,
> > >> and while I think this convention is a bit more sensible, it seems
> > >> somewhat
> > >> unnecessary if variables are properly named. In this context, I think
> > >> it's
> > >> fairly obvious that a variable named "frag" is a packet of some sort
> > >> (whether
> > >> it's an ODP handle or a custom struct or whatever -- distinctions which
> > >> the
> > >> "_pkt" pre/postfix doesn't actually resolve). If there are places in the
> > >> code
> > >> where you specifically think that adding "pkt" would be helpful, though,
> > >> I'd
> > >> be more than happy to consider such changes there.
> > And all functions should have _func/func_ appended/prepended?
> > Just so you know it's a function.
> > >>
> > >
> > > that comment was not strictly say to redo something, it's just my
> > > opinion what is better to read. Thinks like:
> > >
> > > assert(!send_packet(complete_datagram, out));
> > > or
> > > assert(!send_packet(complete_datagram, outq));
>
> I understand it's just your opinion, but I think it's a solution for a
> problem that shouldn't exist in well-written code. (And, at its logical
> conclusion, is madness.)
>
> > >
> > >>> 4. Coding style needs to be corrected.
> > >>
> > >> ???
> > >
> > > mostly variables on top.
>
> (See comments earlier)
>
> > >>> 5. Do you have linaro email address? Can you plese send patch from it.
> > >>
> > >> I had some discussions about this yesterday, and it is my understanding
> > >> that
> > >> I should be able to submit the patch from my ARM email address. Perhaps
> > >> this
> > >> needs some further dialogue, though?
> > >>
> > >>> 6. I'm not really convinced about testing of that example. As future
> > >>> work I think it might be odp helper. And tools like Snort and Suricate
> > >>> can use
> > >>> that odp heper for reassembly instead their own implementation. In that
> > >>> case we need to proove that algorithm work as expected
> > >>> and highlight benefits doing it with ODP. Benefits like faster speed.
> > >>> And proving that it works might be processing pcap file with ip
> > >>> fragments with your
> > >>> program. And as result tcpdump will show no fragmetation on generated
> > >>> pcap. Example also should show that code does not reorder or do other
> > >>> not needed modifications with packets.
> > >>
> > >> Yes, perhaps this would integrate nicely into ODP(H) in future. The
> > >> checking
> > >> performed within the program is fairly decent, and does check the
> > >> reassembled
> > >> packets against the original copies, but I agree that more extensive
> > >> testing
> > >> (including performance tuning and measurement) could provide fertile
> > >> ground
> > >> for future work.
> >
> > If this example is merged, moving relevant pieces into a helper
> > library is easier. We don't need to do all the steps at once. The
> > purpose of this example was to show that the current ODP API was
> > sufficient to do efficient fragmentation and reassembly (we were
> > discussing API additions in this area last year) and to demonstrate a
> > lock-free scalable reassembly design (Linux and FreeBSD kernel
> > reassembly designs use locks etc. we can do better).
> >
> > I think one prerequisite for adding a helper function is that you need
> > the functionality in more than one place. Is there any ODP
> > examples/apps that do their own home grown fragmentation and
> > reassembly today? I would think that OFP should be a primary target
> > for the lock-free reassembly code. Some of the potential/likely use
> > case for OFP will include a lot of fragmented traffic (or so I have
> > been told).
> >
> > >>
> > >
> > > My point is:
> > >
> > > original packet -> wrongly fragmented -> original packet
> > > vs
> > > original packet -> good fragmented -> original packet
> > >
> > > result is the same, middle is different.
> > I don't understand. How do you define wrongly/goodly fragmented?
>
> I think Maxim is saying that if both the fragmentation /and/ reassembly parts
> are broken, then it could end up fudging it such that the result is correct
> even though the individual components (the fragmentation and reassembly
> logic, on their own) are wrong. It's a fair point, and something that should
> be addressed in further testing were this example to be considered a feature
> of ODP(H).
>
> > >
> > >
> > >>> 7. Will you be able to support this app in next 6 months or later?
> > >>
> > >> That depends on your definition of "support". Within the next few months,
> > >> potentially, but much later than that, probably not.
> > >>
> > >
> > > Review new patches. Answer on some community questions which only you
> > > can answer. Something like that....
> > Isn't this what maintainers are for? If you don't understand how the
> > code works, you need to ask more questions or demand better
> > documentation (preferably as comments in the code IMO).
>
> I agree with Ola. I'm doing you guys a favour by contributing a helpful
> example, and would hope not to be shackled to the project by doing so. That
> being said, I appreciate that code contributions are themselves a form of
> technical debt, and will probably be able to answer questions for a few
> months after the contribution if necessary.
