On Thu, 2010-05-20 at 12:22 -0700, Joe Eykholt wrote:
> Vasu Dev wrote:
> > On Wed, 2010-05-19 at 11:16 -0700, Joe Eykholt wrote:
> >>>> */
> >>>> -static void fcoe_ctlr_age_fcfs(struct fcoe_ctlr *fip)
> >>>> +static unsigned long fcoe_ctlr_age_fcfs(struct fcoe_ctlr *fip)
> >>>> {
> >>>> struct fcoe_fcf *fcf;
> >>>> struct fcoe_fcf *next;
> >>>> + unsigned long next_timer = jiffies +
> >> msecs_to_jiffies(FIP_DEF_FKA);
> >>
> >> Also, this should be further out than 8 seconds, in case there's
> >> nothing sooner needed. That's why I used the 90 second time before.
> >> The rest of the code in age_fcfs or the timeout should come up with
> >> a closer value unless the 90 second keep-alive is the next thing
> >> needed
> >> or there's nothing at all to do.
> >>
> >
> > This default value mostly gets written when fd_flag is zero due to
> > sooner E node KA with typical 8 sec KA period and large value was always
> > effective in case fd_flag set that was an issue since in that case
> > periodic ka timeouts didn't kick after 90 second of fcf selection while
> > fd_flag set.
>
> I don't really understand. If there's any FCF at all, it should set
> next_timer to the 1.5 * FKA since the oldest advertisment received.
> With a selected FCF that has the flag on it should fire
> every 1.5 * FKA to check for a missing advertisement.
> With the change above, it'll fire after 1 FKA, find nothing todo
> and then reschedule for .5 FKA later, right?
>
You are right, I must have tried something else in which I noticed
default next_timer used as 90 sec. I don't see with this patch.
> I may have missed something.
>
> > I can roll back this change but then have to set KA period timeout for
> > this case when fcf is selected in fcoe_ctlr_timeout, so it would add few
> > more lines there after fcoe_ctlr_select calling as:-
> >
> > if (sel && sel->fd_flags &&
> > time_after(next_timer, sel->time + sel->fka_period))
> > next_timer = sel->time + fcf->fka_period;
>
> But the code you added in age_fcfs also does something like that:
>
> if (fcf->fd_flags &&
> time_after(next_timer, jiffies +
> fcf->fka_period))
> next_timer = jiffies + fcf->fka_period;
>
> This is after checks that the FCF is selected. It's setting the time
> to now + FKA, not the last advertisement + FKA, though.
> I'm not sure why that's needed, either. Since other code already accounts
> for the 1.5 * FKA check and the 2.5 * FKA timeout.
>
That is to catch missed adv while d flag set as we talked in other
response.
> > I'm fine with either and let me know if you want me to change this.
>
> I would put back the change to initializing next_timer, and take out
> the code quoted above. Is that OK?
OK, but then it won't catch missed adv if none received just after fcf
was selected since TO would be set to 2.5 * FKA in that case. It would
be very unlikely case and would just miss missed adv count and prints.
However if we want that also not to occur then next timer should be
updated just after fcf selected for the D bit case.
Vasu
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
