On Tue, May 10, 2016 at 11:02 AM, Barret Rhoden <[email protected]> wrote:
> Hi - > > On 2016-05-10 at 10:40 Dan Cross <[email protected]> wrote: > > This is hokey in the way we use krefs, due to racey code > > inherited from Plan 9. But it works well enough now that > > ipconfig runs and gets us a DHCP lease. > > > > Change-Id: I966a41f90a2d129eb9f07d9a7993fda990af1dbb > > Google-Bug-Id: 28667887 > > Do you all need this metadata in the commit log? I don't see any > Google-Bug-Id's in upstream Linux's commit log... > I thought I removed them. :-/ > /* > > + * TODO: Change this to a proper release. > > + * At the moment, this is difficult since link removal > > + * requires access to more than just the kref/struct Iplink. > > + * E.g., the self and interface pointers. > > + */ > > +static void link_release(struct kref *kref) > > +{ > > + (void)kref; > > What's this doing? If the function does nothing, we can just have no > statements. This is how C11 spells, "explicitly ignore this parameter, and do not issue a warning if it's unused." > @@ -1027,7 +1038,25 @@ remselfcache(struct Fs *f, struct Ipifc *ifc, > struct Iplifc *lifc, uint8_t * a) > > if (link == NULL) > > panic("remselfcache"); > > > > - if (kref_refcnt(&link->ref) > 1) > > + /* > > + * TODO: The check for 'refcnt > 0' here is awkward. It > > + * exists so that 'remselfcache' can be called concurrently. > > + * In the original plan 9 code, the 'goto out' branch was > > + * taken if the decremented reference count was exactly zero. > > + * In other threads this could become -1, which plan 9 didn't > > + * care about since the logic would be skipped over, and since > > + * 'iplinkfree' won't _actually_ free the link for five seconds > > + * (see comments in that function), and since all of the actual > > + * link manipulation happens in the thread where the reference > > + * is exactly equal to zero. But on Akaros, a negative kref > > + * will panic; hence checking for a positive ref count before > > + * decrementing. This entire mechanism is dubious. But Since > > + * this function is protected by a lock this is probably OK for > > + * the time being. > > + * > > + * However, it is a terrible design and we should fix it. > > Agreed. I almost thing that if we're not going to use krefs properly, > then we shouldn't use them at all. Is link->ref just a counter then? But > agreed, this whole thing is a mess, and this commit is better than nothing. It's essentially an atomic counter. This is probably the least invasive change that gives us "fixed" behavior. > + */ > > + if (kref_refcnt(&link->ref) > 0 && kref_put(&link->ref) != 0) > > goto out; > > > +++ b/kern/src/net/iproute.c > > > +static void route_release(struct kref *kref) > > +{ > > + (void)kref; > > Same as above. > > Barret > > > > -- > You received this message because you are subscribed to the Google Groups > "Akaros" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To post to this group, send email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
