Message-ID: <8a83ec79-be04-ec5c-f3ef-67f64dc55f12@acm.org>
Date:   Tue, 7 Mar 2023 07:47:16 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v4] scsi: ufs: core: Add trace event for MCQ
Content-Language: en-US
To:     Ziqi Chen <quic_ziqichen@quicinc.com>, quic_asutoshd@quicinc.com,
        quic_cang@quicinc.com, mani@kernel.org, stanley.chu@mediatek.com,
        adrian.hunter@intel.com, beanhuo@micron.com, avri.altman@wdc.com,
        junwoo80.lee@samsung.com, martin.petersen@oracle.com
Cc:     linux-scsi@vger.kernel.org, Alim Akhtar <alim.akhtar@samsung.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:TRACING" <linux-trace-kernel@vger.kernel.org>
References: <1677836154-29192-1-git-send-email-quic_ziqichen@quicinc.com>
 <67db2c6b-c3b0-c525-e6a9-2b2fe6c6adbb@acm.org>
 <f80fd91b-3a03-5c38-72c0-cd5c3edb33b8@quicinc.com>
From:   Bart Van Assche <bvanassche@acm.org>
In-Reply-To: <f80fd91b-3a03-5c38-72c0-cd5c3edb33b8@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 3/6/23 21:53, Ziqi Chen wrote:
> You are right,  users may hate it if the trace events for legacy mode 
> and MCQ mode are different. But if I merge them into one event, it will 
> print much invalid information as we can not add if-else into TP_printk().
> 
> (For example:  in SDB legacy mode, you can see such invalid prints " 
> hqid = 0 , sqt= 0, cqh=0, cqt = 0")
> 
> Users may hate these invalid information.
> 
> Anyway, I have made new version that merge 2 mode into one event, but 
> are you sure we really need to use this way? if yes , I can push new 
> version here.
> 
> Or, could you give some suggestions if you have better way.
> 
> Below is a piece of new version code , you can preview.
> 
>      TP_fast_assign(
>          __assign_str(dev_name, dev_name);
>          __entry->str_t = str_t;
>          __entry->tag = tag;
>          __entry->doorbell = doorbell;
>          __entry->hwq_id = hwq? hwq->id: 0;
>          __entry->sq_tail = hwq? hwq->sq_tail_slot: 0;
>          __entry->cq_head = hwq? hwq->cq_head_slot: 0;
>          __entry->cq_tail = hwq? hwq->cq_tail_slot: 0;
>          __entry->transfer_len = transfer_len;
>          __entry->lba = lba;
>          __entry->intr = intr;
>          __entry->opcode = opcode;
>          __entry->group_id = group_id;
>      ),
> 
>      TP_printk(
>          "%s: %s: tag: %u, DB: 0x%x, size: %d, IS: %u, LBA: %llu, 
> opcode: 0x%x (%s),"
>          "group_id: 0x%x, hqid: %d, sqt: %d, cqh: %d, cqt: %d",
>          show_ufs_cmd_trace_str(__entry->str_t), __get_str(dev_name), 
> __entry->tag,
>          __entry->doorbell, __entry->transfer_len, __entry->intr, 
> __entry->lba,
>          (u32)__entry->opcode, str_opcode(__entry->opcode), 
> (u32)__entry->group_id,
>          __entry->hwq_id,__entry->sq_tail, __entry->cq_head,  
> __entry->cq_tail
>      )

Hi Ziqi,

Please reply below the original e-mail instead of above. This is 
expected on Linux kernel mailing lists.

Regarding your question: I propose to leave out the sq_tail, cq_head and 
cq_tail information. That information may be useful for hardware 
developers but is not useful for other users of the Linux kernel. So the 
only piece of information that is left that is MCQ-specific is the 
hardware queue index. I expect that users will be fine to see that 
information in trace events.

How about reporting hardware queue index -1 for legacy mode instead of 
0? That will allow users to tell the difference between legacy mode and 
MCQ mode from the trace events.

Thanks,

Bart.