[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <30c89a45-602f-4fd4-9fe0-70f335859f8f@intel.com>
Date: Mon, 4 Dec 2023 11:26:48 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Jesse Brandeburg <jesse.brandeburg@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
<anthony.l.nguyen@...el.com>, Julia Lawall <julia.lawall@...ia.fr>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1] idpf: refactor some missing
field get/prep conversions
From: Jesse Brandeburg <jesse.brandeburg@...el.com>
Date: Fri, 1 Dec 2023 12:12:05 -0800
> On 12/1/2023 6:32 AM, Alexander Lobakin wrote:
>> From: Jesse Brandeburg <jesse.brandeburg@...el.com>
>
>>> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
>>> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
>>> @@ -505,7 +505,7 @@ static void idpf_rx_post_buf_refill(struct idpf_sw_queue *refillq, u16 buf_id)
>>>
>>> /* store the buffer ID and the SW maintained GEN bit to the refillq */
>>> refillq->ring[nta] =
>>> - ((buf_id << IDPF_RX_BI_BUFID_S) & IDPF_RX_BI_BUFID_M) |
>>> + FIELD_PREP(IDPF_RX_BI_BUFID_M, buf_id) |
>>> (!!(test_bit(__IDPF_Q_GEN_CHK, refillq->flags)) <<
>>> IDPF_RX_BI_GEN_S);
>>
>> Why isn't that one converted as well?
>
> Because it's not a constant, and it's not checking a mask with "&", so
> the automation ignored it. I *did* a test, and we could convert the
> return value from test_bit (a bool) into the IDPF_RX_BI_GEN_M mask with
> FIELD_PREP, since C-code allows the luxury of converting a bool to a
> "1", even though it's a bit type ugly in this age of strict typing.
What is "not a constant"?
ring[nta] = FIELD_PREP(IDPF_RX_BI_GEN_M,
test_bit(__IDPF_Q_GEN_CHK, flags));
Is there a problem with this ^ code?
"The scripts ignored that" is not a good argument I'd say :>
>
>>
>>>
>>> @@ -1825,14 +1825,14 @@ static bool idpf_tx_clean_complq(struct idpf_queue *complq, int budget,
>>> u16 gen;
>>>
>>> /* if the descriptor isn't done, no work yet to do */
>>> - gen = (le16_to_cpu(tx_desc->qid_comptype_gen) &
>>> - IDPF_TXD_COMPLQ_GEN_M) >> IDPF_TXD_COMPLQ_GEN_S;
>>> + gen = FIELD_GET(IDPF_TXD_COMPLQ_GEN_M,
>>> + le16_to_cpu(tx_desc->qid_comptype_gen));
>>
>> The definition:
>>
>> #define IDPF_TXD_COMPLQ_GEN_M BIT_ULL(IDPF_TXD_COMPLQ_GEN_S)
>>
>> Please don't use FIELD_*() API for 1 bit.
>
> Did you mean that gen is effectively used as a bool? I think that has
> nothing to do with my change over to FIELD_GET, but I could see how
> redesigning this code would be useful, but not as part of this
> conversion series.
>
>>
>> gen = !!(le16_to_cpu(tx_desc->qid_comptype_gen) &
>> IDPF_TXD_COMPLQ_GEN_M);
>>
>> is enough.
>
> Generally I'd prefer that the kind of check above returned a bool with a
> constant conversion of the mask (compile time) to an LE16 mask, and then
> use that, which is why all of our other drivers do that instead.
Ah, good point. Smth like
gen = !!(tx_desc->qid_comptype_gen &
IDPF_TXQ_COMPLQ_GEN_M_LE);
OTOH x86 is always LE and BE is seen more and more rarely nowadays. It
might just not worth having a LE-version of each such mask for the sake
of a bit more optimized code on architectures where our drivers are
barely used.
>
>>
>> Moreover, you could use le*_{get,encode,replace}_bits() to get/set LE
>> values right away without 2-step operation (from/to CPU + masks), but
>> you didn't do that here (see below for an example).
>
> Those aren't widely used yet in our drivers so I wasn't picking them up
> yet. But thank you for pointing that out.
>
> <snip>
>
>
>> In general, I would say those two issues are very common in IDPF and
>> also the whole your series converting the Intel drivers. The scripts
>> won't check whether the mask has only 1 bit or whether the value gets
>> converted from/to LE, so they won't help here.
>
> I had been hoping to do some more followup work. it's possible that with
> some tweaking the coccinelle script could learn how to detect non-pow2
> constants, and therefore possibly one bit constants as well. Maybe
> @Julia can help us refine the script and possibly get it into the
> scripts/coccinelle directory to help other drivers as well.
Every automated change needs polishing by human.
Don't FIELD_*() macros already check whether the passed mask is actually
a contiguous mask?
>
>> Could you maybe manually recheck all the places where bitfield masks are
>> used at least in IDPF (better in ice, iavf, i40e, ..., as well) and
>> posted a series that would address them? At the end, manual work is more
>> valuable than automated conversions :p
>
> I think a followup series would work better for this, do you agree?
Nope :D If you want to convert all of our drivers to use bitfield API,
I'd like to see a complete mature series instead of incremental series
of series where followups will refactor the code introduced in the
earlier ones.
>
> Thanks,
> Jesse
Thanks,
Olek
Powered by blists - more mailing lists