On 1 February 2017 at 16:19, Maxim Uvarov <maxim.uva...@linaro.org> wrote:
> On 02/01/17 13:26, Joe Savage wrote:
>> Hey Maxim,
>>
>> I'm adding the mailing list to the CCs.
>>
>
> sorry, looks like I pressed replay instead of replay all.
>
> 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.
>
> 2. Empty braces {} are ok along intend is clear.
>
> 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.
>
>
> answers on questions are bellow.
>
>
>>>> +#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.
> It's very hard to argue because
> different people look from different angle. And what is nice for one
> looks bad for other.. But anyway if you will declare variable on top
> then this question will go away.
>
>
>>> 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.
> 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.._)) {};
>
> }
>
>
>>> 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));
>
>
>>> 4. Coding style needs to be corrected.
>>
>> ???
>
> mostly variables on top.
>
>>
>>> 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?
>
>
>>> 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).
>
> Maxim.
>
>
>
>
>
