Re: Restructuring ARP cache

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Tue, Aug 18, 2015 at 8:03 PM, Christos Zoulas <christos%astron.com@localhost> wrote:
> In article <CAKrYomjz7MjnyAp1xpQQ0z_GQ6i4kUxEbH_8hDq6FuTgdbfHag%mail.gmail.com@localhost>,
> Ryota Ozaki  <ozaki-r%netbsd.org@localhost> wrote:
>>On Mon, Aug 17, 2015 at 6:23 PM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
>>> Hi,
>>>
>>> Here is a new patch that restructures ARP caches, which
>>> aims for MP-safe network stack:
>>> http://www.netbsd.org/~ozaki-r/lltable-arpcache.diff
>>> (https://github.com/ozaki-r/netbsd-src/tree/lltable-arpcache)
>>
>>Updated the patch. The changes are:
>>- Fix freeing mbuf in lltable_drain
>>- Fix rtentry leak
>
> It is hard to do a code review out of context:

I'm sorry for that. As you suspect, lltable codes are untouched
as much as possible for now (if_llatbl.[ch] and in.c).
The changes of if_arp.c are our own and to be reviewed.

> - in_lltable_dump_entry wrong indent?

FreeBSD does so... I'll remove it since it's not used for now.
I hope they fix it by I import when needed.

> - use sizeof(*var) instead of sizeof(type)

Sure. I'll fix it.

> - part of the code has return (foo) and other has return foo. Is that
>   to keep the diffs with FreeBSD mimimal?

Yes. It seems FreeBSD restarted lltable changes recently, so I
keep their codes as is to easily apply their further changes.

BTW, should I KNF the patch before committing whether they would
change soon?

> - foo() {
>         if (....) {
>                 ....
>                 ....
>         }
>   }
>   instead:
>   foo() {
>         if (!....)
>                 return;
>         ....
>         ....
>   }

I prefer to the latter too. Which ones in the patch do you get worried?

> - why cast void *'s for example rif = (struct token_rif *)la->la_opaque;

My fault. I'll fix it.

Thanks,
  ozaki-r