On 02/01/17 18:50, Ola Liljedahl wrote:
> 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.
>
uh, looks like mismatch in understanding... need to sync again.
in v2 there is very few places where it's done. And that is not
necessary to have them in body.
>>
>> 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.
>
yes -latomic helps. It needs to be added to LDFLAGS.
>>
>>
>> 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.
>
Sorry, wanted to write readability.
>> 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).
>
agree, I think very minimal work is needed.
>>>
>>
>> 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?
>
Packet valid or not. All ip headers on their place or shifted, lengths
and checksums are in right byte order, no trail bytes and etc.
>>
>>
>>>> 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).
>
This code is well documented. But we have 2 way review for almost all
patches and I need at least somebody else to review new patches. If
nobody review patch for several week I'm trying to find somebody to review.
>>
>> Maxim.
>>
>>
>>
>>
>>
