[Open-FCoE] [RESUBMIT PATCH] libfc, fcoe: fixed locking issues with lport->lp_mutex around lport->link_status

Joe Eykholt jre at nuovasystems.com
Fri Jan 16 07:36:54 UTC 2009

Vasu Dev wrote:
> Joe Eykholt wrote:
>> Vasu Dev wrote:
>>> -        new_status &= ~FC_LINK_UP;
>>> +        new_link_up = 0;
>>>          if (!fcoe_link_ok(lp))
>>> -            new_status |= FC_LINK_UP;
>>> +            new_link_up = 1;
>> We could say:
>>         new_link_up = !fcoe_link_ok(lp);
>> But it is OK the way you have it.
> I'll make this changes.
>> I think we should change the name of fcoe_link_ok, since it returns 0 
>> when the
>> link is good.  I'd call it fcoe_link_bad(), or have it return 1 when 
>> the link is good.
> I'd prefer returning 1 in this case since that would align well with 
> condition checking on return val
> if (fcoe_link_ok) then
> the link is up;
> else
> the link is down;
> But aren't mostly function suppose to return 0 on success ? So I'd leave 
> this as is for now.

In general, yes, because there's only one success value, but lots of
ways to fail.  But, what is success in this case?   The function is
just returning the link status, not really returning a success/failure 
indication.  Anyway, I think we had that discussion before, so I'll
drop it.

>> <snip>
>>> -    u16            link_status;
>>> +    u32            link_up:1;
>>> +    u32            link_pause:1;
>> We can't use bit fields, because on some architectures to set a bit 
>> field requires a
>> read/modify/write, so we would need a lock that was in common over all 
>> of the bits.
>> It's better to just use two u8 fields here.
> I noticed fc_function_template and fc_lport are already using bit flags 
> at several places, so I went ahead with bit flag, for example:-
> /* host fixed attributes */
> unsigned long show_host_node_name:1;
> unsigned long show_host_port_name:1;
> unsigned long show_host_permanent_port_name:1;
> unsigned long show_host_supported_classes:1;
> unsigned long show_host_supported_fc4s:1;
> unsigned long show_host_supported_speeds:1;
> unsigned long show_host_maxframe_size:1;
> unsigned long show_host_serial_number:1;

Yeah, but these are different because they're set once at initialization
and then left alone.  They don't have to be atomic.  It's not that
bitfields are bad in general, although I know some people don't like
them, but where two bits that could end up in the same byte are
under different locks (or one isn't under a lock) then it's a

> I guess 14 extra bits in this case won't cost any by use of two u8 since 
> now a days memory are cheap :-) if that works better for other 
> architectures. I'll do this change as well.

Thanks, and anyway, the other 14 bits are wasted either way since
the next field will be 32-bit aligned as it happens.

>>     u8 link_up;
>>     u8 link_pause;
>> Umm. Also, it's not really pause, and it isn't a link condition.
>> It's back-pressure, either due to pause or
>> because the NIC can't send as fast as we would like (it's rings are 
>> full).
>> So, could we rename this as 'backpressure' ... maybe later.  At that
> How about qfull for this case ? or may be throttle_enabled.

I like qfull better.

>> point we could also remove the functions to pause/unpause since 
>> they're so trivial.
> This occurred to me as well and in fact I talked to Rob about this 
> before sending out this patch. The only point we had that Scsi_Host 
> doesn't allow changing fields to its lower layer user so may be if 
> possible we shouldn't allow this for lport also though in many case 
> lport and fc_host fields are modified by it user. I'll remove 
> pause/unpause functions also if no more comment on this to have these 
> function on list. I can pull this along this patch re-work.

I guess leaving the functions is fine, I know some lport fields are
accessed directly now, though.   I suppose we shouldn't make too many
unnecessary changes.  We can tweak it forever and not do any real work!

> Thanks Joe for prompt review comments !
> Vasu

You're welcome.


More information about the devel mailing list