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

Vasu Dev 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;
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.
> <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;

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 !
Vasu



More information about the devel mailing list