[B.A.T.M.A.N.] Implement DAT for IPv6

[B.A.T.M.A.N.] Implement DAT for IPv6

[Mihail Costea]

Hi,

I've started implementing DAT for IPv6 and I have a few questions
regarding the design.
I've already implemented a snooping mechanism for Network Solicitation
package, but now I have to add it to the ipv6 distributed table.
I'm also a beginner in kernel programming so any help would be useful.

From what I've seen there is a lot of code in distributed-arp-table.c
that could be used for ipv6 distributed table.
A problem in using that code, from what I've seen, is the fact that
<struct batadv_dat_entry > has a field for ip with only 32 bits
(__be32).

So I was thinking to make it more general by:
1. transforming that into a pointer (unsigned char *ip) and add a
field size which will represent the total size of the ip.
The ip itself would be allocated dynamically.
2. use a union with both _be32 / struct in6_addr. In this case the
problem is that if we use ipv4 then all the other space for struct
in6_addr would be wasted.

The second problem from what I've seen in the code is that the
batadv_priv->dat field is hardcoded.
For ipv6 we would need a new field to memorize the table, but I don't
think it's a really hard problem to solve in order to make it general.

This solution would be to avoid code duplicate. Off course, I could
still write all the code in parallel just for IPv6, but I think that a
common base would be useful.

Thanks,
Mihail

Re: [B.A.T.M.A.N.] Implement DAT for IPv6

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

On Fri, Mar 01, 2013 at 12:59:18PM +0200, Mihail Costea wrote:
> Hi,
> 
> I've started implementing DAT for IPv6 and I have a few questions
> regarding the design.
> I've already implemented a snooping mechanism for Network Solicitation
> package, but now I have to add it to the ipv6 distributed table.
> I'm also a beginner in kernel programming so any help would be useful.
> 
> From what I've seen there is a lot of code in distributed-arp-table.c
> that could be used for ipv6 distributed table.

yeah, I think that the table handling and DHT primitives can be reused for IPv6
(they were designed on purpose).

> A problem in using that code, from what I've seen, is the fact that
> <struct batadv_dat_entry > has a field for ip with only 32 bits
> (__be32).

Yes, here a way to generalise the structure is needed.

> 
> So I was thinking to make it more general by:
> 1. transforming that into a pointer (unsigned char *ip) and add a
> field size which will represent the total size of the ip.
> The ip itself would be allocated dynamically.
> 2. use a union with both _be32 / struct in6_addr. In this case the
> problem is that if we use ipv4 then all the other space for struct
> in6_addr would be wasted.

correct. and for point 2) I think we waste a not negligible amount of memory..

> 
> The second problem from what I've seen in the code is that the
> batadv_priv->dat field is hardcoded.
> For ipv6 we would need a new field to memorize the table, but I don't
> think it's a really hard problem to solve in order to make it general.
> 

I did not get the problem with the dat field?
Can't you use the same table to store both IPv4 and IPv6? The DHT is made to
store general data, so I think (with the suggestions you wrote above) that you
should be able to use one table only.

> This solution would be to avoid code duplicate. Off course, I could
> still write all the code in parallel just for IPv6, but I think that a
> common base would be useful.
> 

Re-use code is definitely the way to go :) easy maintenance later, one single
point for bug-fixing and code has been tested already (so it should work[tm] as
expected :-))

> Thanks,

no worries :)

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] Implement DAT for IPv6

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 634 bytes --]

On Fri, Mar 01, 2013 at 12:59:18PM +0200, Mihail Costea wrote:
> Hi,
> 
> I've started implementing DAT for IPv6 and I have a few questions
> regarding the design.
> I've already implemented a snooping mechanism for Network Solicitation
> package, 

I forgot to ask here: packets generated by the IPv6 the Neigh Discovery Protocol
are sent on a periodic basis, right? so what is the idea behind the snooping
mechanism? There is not a request (like for ARP), right? So when should the data
in distributed table be used?

Cheers,


-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] Implement DAT for IPv6

[Mihail Costea]

On 1 March 2013 13:08, Antonio Quartulli <ordex@autistici.org> wrote:
> On Fri, Mar 01, 2013 at 12:59:18PM +0200, Mihail Costea wrote:
>> Hi,
>>
>> I've started implementing DAT for IPv6 and I have a few questions
>> regarding the design.
>> I've already implemented a snooping mechanism for Network Solicitation
>> package, but now I have to add it to the ipv6 distributed table.
>> I'm also a beginner in kernel programming so any help would be useful.
>>
>> From what I've seen there is a lot of code in distributed-arp-table.c
>> that could be used for ipv6 distributed table.
>
> yeah, I think that the table handling and DHT primitives can be reused for IPv6
> (they were designed on purpose).
>
>> A problem in using that code, from what I've seen, is the fact that
>> <struct batadv_dat_entry > has a field for ip with only 32 bits
>> (__be32).
>
> Yes, here a way to generalise the structure is needed.
>
>>
>> So I was thinking to make it more general by:
>> 1. transforming that into a pointer (unsigned char *ip) and add a
>> field size which will represent the total size of the ip.
>> The ip itself would be allocated dynamically.
>> 2. use a union with both _be32 / struct in6_addr. In this case the
>> problem is that if we use ipv4 then all the other space for struct
>> in6_addr would be wasted.
>
> correct. and for point 2) I think we waste a not negligible amount of memory..
>
>>
>> The second problem from what I've seen in the code is that the
>> batadv_priv->dat field is hardcoded.
>> For ipv6 we would need a new field to memorize the table, but I don't
>> think it's a really hard problem to solve in order to make it general.
>>
>
> I did not get the problem with the dat field?
> Can't you use the same table to store both IPv4 and IPv6? The DHT is made to
> store general data, so I think (with the suggestions you wrote above) that you
> should be able to use one table only.
>

I think it can be reuse. If I understood well the DHT has buckets for
each entry, so it shouldn't be a problem if
the IPv6 hash collides with the IPv4 hash.
I just thought that they should be separated because because they are
different concepts.
This makes it a lot more easier :).

>> This solution would be to avoid code duplicate. Off course, I could
>> still write all the code in parallel just for IPv6, but I think that a
>> common base would be useful.
>>
>
> Re-use code is definitely the way to go :) easy maintenance later, one single
> point for bug-fixing and code has been tested already (so it should work[tm] as
> expected :-))
>
>> Thanks,
>
> no worries :)
>
> Cheers,
>
> --
> Antonio Quartulli
>
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara

Re: [B.A.T.M.A.N.] Implement DAT for IPv6

[Mihail Costea]

On 1 March 2013 13:11, Antonio Quartulli <ordex@autistici.org> wrote:
> On Fri, Mar 01, 2013 at 12:59:18PM +0200, Mihail Costea wrote:
>> Hi,
>>
>> I've started implementing DAT for IPv6 and I have a few questions
>> regarding the design.
>> I've already implemented a snooping mechanism for Network Solicitation
>> package,
>
> I forgot to ask here: packets generated by the IPv6 the Neigh Discovery Protocol
> are sent on a periodic basis, right? so what is the idea behind the snooping
> mechanism? There is not a request (like for ARP), right? So when should the data
> in distributed table be used?

From what I understood the Neighbor Solicitation and Neighbor
Advertisement packets are the ARP equivalents.
So if we receive a Neighbor Solicitation than we would use the
distributed table and send the Neighbor Advertisement directly.

>
> Cheers,
>
>
> --
> Antonio Quartulli
>
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara

Thanks,
Mihail

Re: [B.A.T.M.A.N.] Implement DAT for IPv6

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 3007 bytes --]

On Fri, Mar 01, 2013 at 01:38:43PM +0200, Mihail Costea wrote:
> On 1 March 2013 13:08, Antonio Quartulli <ordex@autistici.org> wrote:
> > On Fri, Mar 01, 2013 at 12:59:18PM +0200, Mihail Costea wrote:
> >> Hi,
> >>
> >> I've started implementing DAT for IPv6 and I have a few questions
> >> regarding the design.
> >> I've already implemented a snooping mechanism for Network Solicitation
> >> package, but now I have to add it to the ipv6 distributed table.
> >> I'm also a beginner in kernel programming so any help would be useful.
> >>
> >> From what I've seen there is a lot of code in distributed-arp-table.c
> >> that could be used for ipv6 distributed table.
> >
> > yeah, I think that the table handling and DHT primitives can be reused for IPv6
> > (they were designed on purpose).
> >
> >> A problem in using that code, from what I've seen, is the fact that
> >> <struct batadv_dat_entry > has a field for ip with only 32 bits
> >> (__be32).
> >
> > Yes, here a way to generalise the structure is needed.
> >
> >>
> >> So I was thinking to make it more general by:
> >> 1. transforming that into a pointer (unsigned char *ip) and add a
> >> field size which will represent the total size of the ip.
> >> The ip itself would be allocated dynamically.
> >> 2. use a union with both _be32 / struct in6_addr. In this case the
> >> problem is that if we use ipv4 then all the other space for struct
> >> in6_addr would be wasted.
> >
> > correct. and for point 2) I think we waste a not negligible amount of memory..
> >
> >>
> >> The second problem from what I've seen in the code is that the
> >> batadv_priv->dat field is hardcoded.
> >> For ipv6 we would need a new field to memorize the table, but I don't
> >> think it's a really hard problem to solve in order to make it general.
> >>
> >
> > I did not get the problem with the dat field?
> > Can't you use the same table to store both IPv4 and IPv6? The DHT is made to
> > store general data, so I think (with the suggestions you wrote above) that you
> > should be able to use one table only.
> >
> 
> I think it can be reuse. If I understood well the DHT has buckets for
> each entry, so it shouldn't be a problem if
> the IPv6 hash collides with the IPv4 hash.

yeah in general hash collisions are not a problem (well it is better to reduce
them in order to provide a better distribution of the data).

> I just thought that they should be separated because because they are
> different concepts.

yes, but in the end they are still separated at the logical level (the data 
size will make the difference). Maybe you can add a "type" field on the bucket?
In this way we can easily differentiate one data type from another (this maybe
helpful for future use of the DHT too) instead of replying only on the
"addr_size" field.

> This makes it a lot more easier :).

Yeah :)

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] Implement DAT for IPv6

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

On Fri, Mar 01, 2013 at 01:41:28PM +0200, Mihail Costea wrote:
> On 1 March 2013 13:11, Antonio Quartulli <ordex@autistici.org> wrote:
> > On Fri, Mar 01, 2013 at 12:59:18PM +0200, Mihail Costea wrote:
> >> Hi,
> >>
> >> I've started implementing DAT for IPv6 and I have a few questions
> >> regarding the design.
> >> I've already implemented a snooping mechanism for Network Solicitation
> >> package,
> >
> > I forgot to ask here: packets generated by the IPv6 the Neigh Discovery Protocol
> > are sent on a periodic basis, right? so what is the idea behind the snooping
> > mechanism? There is not a request (like for ARP), right? So when should the data
> > in distributed table be used?
> 
> From what I understood the Neighbor Solicitation and Neighbor
> Advertisement packets are the ARP equivalents.
> So if we receive a Neighbor Solicitation than we would use the
> distributed table and send the Neighbor Advertisement directly.

Oh ok, It was my fault. I quickly re-read part of the RFC4861: only router
advertisement are sent on a periodic basis, while the neighbour advertisement is
asynchronously sent either on address change or as a reply to a neighbour
solicitation.

Thanks.

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] Implement DAT for IPv6

[Mihail Costea]

On 1 March 2013 13:43, Antonio Quartulli <ordex@autistici.org> wrote:
> On Fri, Mar 01, 2013 at 01:38:43PM +0200, Mihail Costea wrote:
>> On 1 March 2013 13:08, Antonio Quartulli <ordex@autistici.org> wrote:
>> > On Fri, Mar 01, 2013 at 12:59:18PM +0200, Mihail Costea wrote:
>> >> Hi,
>> >>
>> >> I've started implementing DAT for IPv6 and I have a few questions
>> >> regarding the design.
>> >> I've already implemented a snooping mechanism for Network Solicitation
>> >> package, but now I have to add it to the ipv6 distributed table.
>> >> I'm also a beginner in kernel programming so any help would be useful.
>> >>
>> >> From what I've seen there is a lot of code in distributed-arp-table.c
>> >> that could be used for ipv6 distributed table.
>> >
>> > yeah, I think that the table handling and DHT primitives can be reused for IPv6
>> > (they were designed on purpose).
>> >
>> >> A problem in using that code, from what I've seen, is the fact that
>> >> <struct batadv_dat_entry > has a field for ip with only 32 bits
>> >> (__be32).
>> >
>> > Yes, here a way to generalise the structure is needed.
>> >
>> >>
>> >> So I was thinking to make it more general by:
>> >> 1. transforming that into a pointer (unsigned char *ip) and add a
>> >> field size which will represent the total size of the ip.
>> >> The ip itself would be allocated dynamically.
>> >> 2. use a union with both _be32 / struct in6_addr. In this case the
>> >> problem is that if we use ipv4 then all the other space for struct
>> >> in6_addr would be wasted.
>> >
>> > correct. and for point 2) I think we waste a not negligible amount of memory..
>> >
>> >>
>> >> The second problem from what I've seen in the code is that the
>> >> batadv_priv->dat field is hardcoded.
>> >> For ipv6 we would need a new field to memorize the table, but I don't
>> >> think it's a really hard problem to solve in order to make it general.
>> >>
>> >
>> > I did not get the problem with the dat field?
>> > Can't you use the same table to store both IPv4 and IPv6? The DHT is made to
>> > store general data, so I think (with the suggestions you wrote above) that you
>> > should be able to use one table only.
>> >
>>
>> I think it can be reuse. If I understood well the DHT has buckets for
>> each entry, so it shouldn't be a problem if
>> the IPv6 hash collides with the IPv4 hash.
>
> yeah in general hash collisions are not a problem (well it is better to reduce
> them in order to provide a better distribution of the data).
>
>> I just thought that they should be separated because because they are
>> different concepts.
>
> yes, but in the end they are still separated at the logical level (the data
> size will make the difference). Maybe you can add a "type" field on the bucket?
> In this way we can easily differentiate one data type from another (this maybe
> helpful for future use of the DHT too) instead of replying only on the
> "addr_size" field.
>

Yes, the type is a better idea.
Then, back to coding :).

Cheers,
Mihail

Re: [B.A.T.M.A.N.] Implement DAT for IPv6

[Mihail Costea]

Hi,

I have created a patch for this, just that is quite big (~ 450 lines
the whole patch, with signature, etc). It doesn't contain complex
changes, just small changes from __be32 to (unsigned char *), a new
parameter to functions in order to give the IP type and added
comments.

I've tested it with the snooping mechanism I added for IPv6 and it
works ok (the snooping mechanism is not in this patch).

What I wanted to ask is: it's ok to send a patch this big to the mailing list?
I don't see how I could split it more because all I did is changed
most of functions signatures and edit the code to support this
changes.

Thx,
Mihail

Re: [B.A.T.M.A.N.] Implement DAT for IPv6

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On Fri, Mar 08, 2013 at 11:22:37AM +0200, Mihail Costea wrote:
> Hi,
> 
> I have created a patch for this, just that is quite big (~ 450 lines
> the whole patch, with signature, etc). It doesn't contain complex
> changes, just small changes from __be32 to (unsigned char *), a new
> parameter to functions in order to give the IP type and added
> comments.
> 
> I've tested it with the snooping mechanism I added for IPv6 and it
> works ok (the snooping mechanism is not in this patch).
> 
> What I wanted to ask is: it's ok to send a patch this big to the mailing list?
> I don't see how I could split it more because all I did is changed
> most of functions signatures and edit the code to support this
> changes.


The common rule is "one change per patch" meaning that if you can logically
split it (e.g. 1) add this 2) use this there and there 3) improve that) it would
be better to send more than one patch in one patchset.
Remember that each and every patch must compile.

Cheers,

p.s. if you want you can send the patch as RFC and see the others say (but
please try to think about splitting it, if really needed).

> 
> Thx,
> Mihail

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] Implement DAT for IPv6

[Mihail Costea]

On 8 March 2013 11:33, Antonio Quartulli <ordex@autistici.org> wrote:
> On Fri, Mar 08, 2013 at 11:22:37AM +0200, Mihail Costea wrote:
>> Hi,
>>
>> I have created a patch for this, just that is quite big (~ 450 lines
>> the whole patch, with signature, etc). It doesn't contain complex
>> changes, just small changes from __be32 to (unsigned char *), a new
>> parameter to functions in order to give the IP type and added
>> comments.
>>
>> I've tested it with the snooping mechanism I added for IPv6 and it
>> works ok (the snooping mechanism is not in this patch).
>>
>> What I wanted to ask is: it's ok to send a patch this big to the mailing list?
>> I don't see how I could split it more because all I did is changed
>> most of functions signatures and edit the code to support this
>> changes.
>
>
> The common rule is "one change per patch" meaning that if you can logically
> split it (e.g. 1) add this 2) use this there and there 3) improve that) it would
> be better to send more than one patch in one patchset.
> Remember that each and every patch must compile.

I think I could split it into 2-3 parts logically:
1. Add the enum for IP types and add generic functions for comparing,
calculating hash
2. Add generic debug message
3. Modify batadv_dat_entry  and all other functions to support both IPv4/IPv6

Some functions from 1./2. might not be used from start, but only
later, when 3. is done.

>
> Cheers,
>
> p.s. if you want you can send the patch as RFC and see the others say (but
> please try to think about splitting it, if really needed).
>
>>
>> Thx,
>> Mihail
>
> --
> Antonio Quartulli
>
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara



-- 
Mihail Costea
E-mail : mihail.costea90@gmail.com
Telephone : +40725245085

Re: [B.A.T.M.A.N.] Implement DAT for IPv6

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]

On Fri, Mar 08, 2013 at 12:11:42PM +0200, Mihail Costea wrote:
> On 8 March 2013 11:33, Antonio Quartulli <ordex@autistici.org> wrote:
> > On Fri, Mar 08, 2013 at 11:22:37AM +0200, Mihail Costea wrote:
> >> Hi,
> >>
> >> I have created a patch for this, just that is quite big (~ 450 lines
> >> the whole patch, with signature, etc). It doesn't contain complex
> >> changes, just small changes from __be32 to (unsigned char *), a new
> >> parameter to functions in order to give the IP type and added
> >> comments.
> >>
> >> I've tested it with the snooping mechanism I added for IPv6 and it
> >> works ok (the snooping mechanism is not in this patch).
> >>
> >> What I wanted to ask is: it's ok to send a patch this big to the mailing list?
> >> I don't see how I could split it more because all I did is changed
> >> most of functions signatures and edit the code to support this
> >> changes.
> >
> >
> > The common rule is "one change per patch" meaning that if you can logically
> > split it (e.g. 1) add this 2) use this there and there 3) improve that) it would
> > be better to send more than one patch in one patchset.
> > Remember that each and every patch must compile.
> 
> I think I could split it into 2-3 parts logically:
> 1. Add the enum for IP types and add generic functions for comparing,
> calculating hash
> 2. Add generic debug message
> 3. Modify batadv_dat_entry  and all other functions to support both IPv4/IPv6
> 
> Some functions from 1./2. might not be used from start, but only
> later, when 3. is done.

Well, looking at the code it is easier to judge :)
Feel free to send your RFC/PATCH to the ml.

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]