[Open-FCoE] [RESUBMIT PATCH] libfc, fcoe: fixed locking issues with lport->lp_mutex around lport->link_status
vasu.dev at linux.intel.com
Fri Jan 16 04:28:49 UTC 2009
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;
the link is down;
But aren't mostly function suppose to return 0 on success ? So I'd leave
this as is for now.
>> - 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;
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.
> 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.
> 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.
Thanks Joe for prompt review comments !
More information about the devel