Joe Eykholt wrote:
> Vasu Dev wrote:
>> From: Joe Eykholt <[email protected]>
>>
>> This patch has several improvements to the code in
>> the fip timers.
>>
>> The current code allows an advertisement to be used
>> even if it has been 3 times the FCF keep-alive
>> advertisement period (FKA) since one was received from
>> that FCF. The spec. calls for 2.5 times FKA.
>>
>> Fix this and make sure we detect missed keep-alives promptly.
>>
>> Signed-off-by: Joe Eykholt <[email protected]>
>>
>> --
>> Use fka_period as periodic timeouts to age out fcf if
>> keep alives are disabled due to fd_flags set and also
>> stop updating keep alive values in that case.
>>
>> Update select fcf time only if fcf is not already selected or
>> select time is not already determined from parse adv, and then
>> have select time cleared only once after fcf is selected.
>>
>> Changed deadline check to time_after_eq() from time_after()
>> since now next timeout will be on exact 2.5 times FKA followed
>> by first advertisement.
>>
>> Signed-off-by: Vasu Dev <[email protected]>
>> ---
>>
>> drivers/scsi/fcoe/libfcoe.c | 89
>> ++++++++++++++++++++++++-------------------
>> 1 files changed, 50 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/scsi/fcoe/libfcoe.c b/drivers/scsi/fcoe/libfcoe.c
>> index 1d5b949..6ac1f0e 100644
>> --- a/drivers/scsi/fcoe/libfcoe.c
>> +++ b/drivers/scsi/fcoe/libfcoe.c
>> @@ -557,38 +557,48 @@ EXPORT_SYMBOL(fcoe_ctlr_els_send);
>> *
>> * Called with lock held and preemption disabled.
>> *
>> - * An FCF is considered old if we have missed three advertisements.
>> - * That is, there have been no valid advertisement from it for three
>> - * times its keep-alive period including fuzz.
>> + * An FCF is considered old if we have missed two advertisements.
>> + * That is, there have been no valid advertisement from it for 2.5
>> + * times its keep-alive period.
>> *
>> * In addition, determine the time when an FCF selection can occur.
>> *
>> * Also, increment the MissDiscAdvCount when no advertisement is received
>> * for the corresponding FCF for 1.5 * FKA_ADV_PERIOD (FC-BB-5 LESB).
>> + *
>> + * Returns the time in jiffies for the next call.
>> */
>> -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.
>> + unsigned long deadline;
>> unsigned long sel_time = 0;
>> - unsigned long mda_time = 0;
>> struct fcoe_dev_stats *stats;
>>
>> list_for_each_entry_safe(fcf, next, &fip->fcfs, list) {
>> - mda_time = fcf->fka_period + (fcf->fka_period >> 1);
>> - if ((fip->sel_fcf == fcf) &&
>> - (time_after(jiffies, fcf->time + mda_time))) {
>> - mod_timer(&fip->timer, jiffies + mda_time);
>> - stats = per_cpu_ptr(fip->lp->dev_stats,
>> - smp_processor_id());
>> - stats->MissDiscAdvCount++;
>> - printk(KERN_INFO "libfcoe: host%d: Missing Discovery "
>> - "Advertisement for fab %16.16llx count %lld\n",
>> - fip->lp->host->host_no, fcf->fabric_name,
>> - stats->MissDiscAdvCount);
>
>
>> + deadline = fcf->time + fcf->fka_period + fcf->fka_period / 2;
>> + if (fip->sel_fcf == fcf) {
>> + if (fcf->fd_flags &&
>> + time_after(next_timer, jiffies + fcf->fka_period))
>> + next_timer = jiffies + fcf->fka_period;
>
> The fd_flags should be in the outer if, since that means we shouldn't
> be looking for missing advertisements at all. Otherwise we'll set
> next_timer to the deadline.
>
> if (fip->sel_fcf == fcf && !fcf->fd_flags) {
> if (time_after ...
>
>> +
>> + if (time_after(jiffies, deadline)) {
>> + stats = per_cpu_ptr(fip->lp->dev_stats,
>> + smp_processor_id());
>> + stats->MissDiscAdvCount++;
>> + printk(KERN_INFO "libfcoe: host%d: "
>> + "Missing Discovery Advertisement "
>> + "for fab %16.16llx count %lld\n",
>> + fip->lp->host->host_no, fcf->fabric_name,
>> + stats->MissDiscAdvCount);
>> + } else if (time_after(next_timer, deadline))
>> + next_timer = deadline;
>> }
>> - if (time_after(jiffies, fcf->time + fcf->fka_period * 3 +
>> - msecs_to_jiffies(FIP_FCF_FUZZ * 3))) {
>> +
>> + deadline += fcf->fka_period;
>> + if (time_after_eq(jiffies, deadline)) {
>
> This is a pre-existing bug, but the above also should check
> fcf->fd_flags around both the if and else so
>
> if (!fcf->fd_flags) {
> if (time_after_eq(...)) {
> ...
> delete the fcf
>
> continue; // so we don't check sel_time on this
> one.
> }
> if (time_after(next_timer, deadline))
> next_timer = deadline;
> }
> if (fcoe_ctlr_mtu_valid(fcf) && ...
>
> This rearranges things a bit. I hope you see what I mean.
> Let me know if you want to split that out into a separate
> patch or want me to update this patch.
>
>> if (fip->sel_fcf == fcf)
>> fip->sel_fcf = NULL;
>> list_del(&fcf->list);
>> @@ -598,19 +608,20 @@ static void fcoe_ctlr_age_fcfs(struct fcoe_ctlr *fip)
>> stats = per_cpu_ptr(fip->lp->dev_stats,
>> smp_processor_id());
>> stats->VLinkFailureCount++;
>> - } else if (fcoe_ctlr_mtu_valid(fcf) &&
>> - (!sel_time || time_before(sel_time, fcf->time))) {
>> - sel_time = fcf->time;
>> + } else {
>> + if (time_after(next_timer, deadline))
>> + next_timer = deadline;
>> + if (fcoe_ctlr_mtu_valid(fcf) &&
>> + (!sel_time || time_before(sel_time, fcf->time)))
>> + sel_time = fcf->time;
>> }
>> }
>> - if (sel_time) {
>> + if (!(fip->sel_fcf || fip->sel_time)) {
>
> Shouldn't this be:
> if (sel_time && !fip->sel_fcf) {
>
> sel_time non-zero indicates that one of the FCFs in the loop
>
>> sel_time += msecs_to_jiffies(FCOE_CTLR_START_DELAY);
>> fip->sel_time = sel_time;
>> - if (time_before(sel_time, fip->timer.expires))
>> - mod_timer(&fip->timer, sel_time);
>> - } else {
>> - fip->sel_time = 0;
>> }
>> +
>> + return next_timer;
>> }
>>
>> /**
>> @@ -767,7 +778,7 @@ static void fcoe_ctlr_recv_adv(struct fcoe_ctlr *fip,
>> struct sk_buff *skb)
>> * ignored after a usable solicited advertisement
>> * has been received.
>> */
>> - if (fcf == fip->sel_fcf) {
>> + if (fcf == fip->sel_fcf && !fcf->fd_flags) {
>> fip->ctlr_ka_time -= fcf->fka_period;
>> fip->ctlr_ka_time += new.fka_period;
>> if (time_before(fip->ctlr_ka_time, fip->timer.expires))
>> @@ -805,7 +816,7 @@ static void fcoe_ctlr_recv_adv(struct fcoe_ctlr *fip,
>> struct sk_buff *skb)
>> * If this is the first validated FCF, note the time and
>> * set a timer to trigger selection.
>> */
>> - if (mtu_valid && !fip->sel_time && fcoe_ctlr_fcf_usable(fcf)) {
>> + if (mtu_valid && !fip->sel_fcf && fcoe_ctlr_fcf_usable(fcf)) {
>> fip->sel_time = jiffies +
>> msecs_to_jiffies(FCOE_CTLR_START_DELAY);
>> if (!timer_pending(&fip->timer) ||
>> @@ -1148,7 +1159,7 @@ static void fcoe_ctlr_timeout(unsigned long arg)
>> struct fcoe_ctlr *fip = (struct fcoe_ctlr *)arg;
>> struct fcoe_fcf *sel;
>> struct fcoe_fcf *fcf;
>> - unsigned long next_timer = jiffies + msecs_to_jiffies(FIP_VN_KA_PERIOD);
>> + unsigned long next_timer;
>>
>> spin_lock_bh(&fip->lock);
>> if (fip->state == FIP_ST_DISABLED) {
>> @@ -1157,13 +1168,16 @@ static void fcoe_ctlr_timeout(unsigned long arg)
>> }
>>
>> fcf = fip->sel_fcf;
>> - fcoe_ctlr_age_fcfs(fip);
>> + next_timer = fcoe_ctlr_age_fcfs(fip);
>>
>> sel = fip->sel_fcf;
>> - if (!sel && fip->sel_time && time_after_eq(jiffies, fip->sel_time)) {
>> - fcoe_ctlr_select(fip);
>> - sel = fip->sel_fcf;
>> - fip->sel_time = 0;
>> + if (!sel && fip->sel_time) {
>> + if (time_after_eq(jiffies, fip->sel_time)) {
>> + fcoe_ctlr_select(fip);
>> + sel = fip->sel_fcf;
>> + fip->sel_time = 0;
>> + } else if (time_after(next_timer, fip->sel_time))
>> + next_timer = fip->sel_time;
>> }
>>
>> if (sel != fcf) {
>> @@ -1201,12 +1215,9 @@ static void fcoe_ctlr_timeout(unsigned long arg)
>> }
>> if (time_after(next_timer, fip->port_ka_time))
>> next_timer = fip->port_ka_time;
>> - mod_timer(&fip->timer, next_timer);
>> - } else if (fip->sel_time) {
>> - next_timer = fip->sel_time +
>> - msecs_to_jiffies(FCOE_CTLR_START_DELAY);
>> - mod_timer(&fip->timer, next_timer);
>> }
>> + if (!list_empty(&fip->fcfs))
>> + mod_timer(&fip->timer, next_timer);
>> if (fip->send_ctlr_ka || fip->send_port_ka)
>> schedule_work(&fip->timer_work);
>> spin_unlock_bh(&fip->lock);
>>
>> _______________________________________________
>> devel mailing list
>> [email protected]
>> http://www.open-fcoe.org/mailman/listinfo/devel
>
> _______________________________________________
> devel mailing list
> [email protected]
> http://www.open-fcoe.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
