On Fri, Jun 24, 2022 at 05:03:23PM -0700, Gerd Rausch wrote:
> Hi Jason,
>
> On Fri, 2022-06-24 at 20:11 -0300, Jason Gunthorpe wrote:
> > On Thu, Jun 16, 2022 at 08:57:14AM -0700, Gerd Rausch wrote:
> > > Unlike with IPv[46], where "ip_finish_output2" triggers
> > > a refresh of STALE neighbour entries via "neigh_output",
> > > "rdma_resolve_addr" never triggers an update.
> >
> > Why? We alread call neigh_event_send() right after this in
> > addr_resolve_neigh(), and this seems to ignore the input resolve_neigh
> > to addr_resolve() ?
> >
>
> This in "dst_fetch_ha"?:
> --------%<--------%<--------%<--------%<--------%<--------
> if (!(n->nud_state & NUD_VALID)) {
> neigh_event_send(n, NULL);
> ret = -ENODATA;
> --------%<--------%<--------%<--------%<--------%<--------
>
> With:
> #define NUD_VALID (NUD_PERMANENT|NUD_NOARP|NUD_REACHABLE|NUD_PROBE
> |NUD_STALE|NUD_DELAY)
>
> and the knowledge that the ARP entry is NUD_STALE,
> how would that function be called and perform the necessary refresh?
>
>
> > I think there is more going on here than this commit message is
> > explaining.
> >
>
> If the intention of the above mentioned "neigh_event_send" was to
> refresh stale entries, then the "if" condition ought to change, no?
Maybe yes.
If you are saying with this patch that neigh_event_send() should be
called unconditionally for every 'packet' why not remove the test
above instead of calling it twice?
On the other hand I see many places checking for NUD_VALID before
calling it.
So, really the commit message needs to explain how neigh_event_send()
is supposed to be used and explain that NUD_VALID is OK to drop.
I'm having a hard time guessing from a quick look around.
Jason
