[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 17:05:14 UTC 2009


Joe Eykholt wrote:
>
> 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
> problem.
>

I see, good points.

>> 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.
>

true.

>>>     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.
>

OK, will go with this name.

>>> 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'll removed these functions since  since fc_pause/fc_unpause won't 
align well to qfull name, a simple change in this case.

> I suppose we shouldn't make too many
> unnecessary changes.  We can tweak it forever and not do any real work!
>

I agree since  unnecessary changes like  name changes and moving code 
pieces around doesn't do any real work. However above suggested naming 
change qfull and removing two one liner functions will go well along 
this  patch which also fixed bugs.

    Regards
    Vasu




More information about the devel mailing list