On 8 February 2017 at 12:15, Joe Savage <joe.sav...@arm.com> wrote:
> Any further thoughts on these issues, Maxim? I'm keen to get this patch
> merged already!
>
>
No, only which I wrote before. Main comment is that we need it compile in
our CI env,
so you need to add atomic library as Ola wrote. And few places where
variables need go
on top.
Maxim.
> > > > 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.
>
