From: Hannes Reinecke <hare@suse.de>
To: James Smart <jsmart2021@gmail.com>,
Muneendra <muneendra.kumar@broadcom.com>,
linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
tj@kernel.org, linux-nvme@lists.infradead.org
Cc: emilne@redhat.com, mkumar@redhat.com,
Gaurav Srivastava <gaurav.srivastava@broadcom.com>
Subject: Re: [PATCH v9 13/13] lpfc: vmid: Introducing vmid in io path.
Date: Mon, 12 Apr 2021 07:27:14 +0200 [thread overview]
Message-ID: <edbcc321-fba1-e908-7932-ecb6b70ffc50@suse.de> (raw)
In-Reply-To: <201c6604-8e9d-bfcb-f39e-be507f8b02d6@gmail.com>
On 4/10/21 5:00 PM, James Smart wrote:
> On 4/8/2021 1:46 AM, Hannes Reinecke wrote:
>> On 4/7/21 1:06 AM, Muneendra wrote:
>>> From: Gaurav Srivastava <gaurav.srivastava@broadcom.com>
> ...
>>> + /* while the read lock was released, in case the entry was */
>>> + /* added by other context or is in process of being added */
>>> + if (vmp && vmp->flag & LPFC_VMID_REGISTERED) {
>>> + lpfc_vmid_update_entry(vport, cmd, vmp, tag);
>>> + write_unlock(&vport->vmid_lock);
>>> + return 0;
>>> + } else if (vmp && vmp->flag & LPFC_VMID_REQ_REGISTER) {
>>> + write_unlock(&vport->vmid_lock);
>>> + return -EBUSY;
>>> + }
>>> +
>>> + /* else search and allocate a free slot in the hash table */
>>> + if (vport->cur_vmid_cnt < vport->max_vmid) {
>>> + for (i = 0; i < vport->max_vmid; ++i) {
>>> + vmp = vport->vmid + i;
>>> + if (vmp->flag == LPFC_VMID_SLOT_FREE) {
>>> + vmp = vport->vmid + i;
>
> delete this last line and adjust parens - really odd that it replicates
> the assignment 2 lines earlier.
>
>>> + break;
>>> + }
>>> + }
>
> I would prefer that if the table is expended and no slots free, that
> -ENOMEM is returned here. Rather than falling down below and qualifying
> by slot free, then by pending (set only if slot free). I can't believe
> there is a reason the idle timer has to be started if no slots free as
> all the other fail cases don't bother with it.
>
> This also helps indentation levels below.
>
>>> + } else {
>>> + write_unlock(&vport->vmid_lock);
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + if (vmp && (vmp->flag == LPFC_VMID_SLOT_FREE)) {
>>> + /* Add the vmid and register */
>>> + lpfc_put_vmid_in_hashtable(vport, hash, vmp);
>>> + vmp->vmid_len = len;
>>> + memcpy(vmp->host_vmid, uuid, vmp->vmid_len);
>>> + vmp->io_rd_cnt = 0;
>>> + vmp->io_wr_cnt = 0;
>>> + vmp->flag = LPFC_VMID_SLOT_USED;
>>> +
>>> + vmp->delete_inactive =
>>> + vport->vmid_inactivity_timeout ? 1 : 0;
>>> +
>>> + /* if type priority tag, get next available vmid */
>>> + if (lpfc_vmid_is_type_priority_tag(vport))
>>> + lpfc_vmid_assign_cs_ctl(vport, vmp);
>>> +
>>> + /* allocate the per cpu variable for holding */
>>> + /* the last access time stamp only if vmid is enabled */
>>> + if (!vmp->last_io_time)
>>> + vmp->last_io_time =
>>> + __alloc_percpu(sizeof(u64),
>>> + __alignof__(struct
>>> + lpfc_vmid));
>>> +
>>> + /* registration pending */
>>> + pending = 1;
>>> + } else {
>>> + rc = -ENOMEM;
>>> + }
>>> + write_unlock(&vport->vmid_lock);
>>> +
>>> + /* complete transaction with switch */
>>> + if (pending) {
>>> + if (lpfc_vmid_is_type_priority_tag(vport))
>>> + rc = lpfc_vmid_uvem(vport, vmp, true);
>>> + else
>>> + rc = lpfc_vmid_cmd(vport,
>>> + SLI_CTAS_RAPP_IDENT,
>>> + vmp);
>>> + if (!rc) {
>>> + write_lock(&vport->vmid_lock);
>>> + vport->cur_vmid_cnt++;
>>> + vmp->flag |= LPFC_VMID_REQ_REGISTER;
>>> + write_unlock(&vport->vmid_lock);
>>> + }
>>> + }
>>> +
>>> + /* finally, enable the idle timer once */
>>> + if (!(vport->phba->pport->vmid_flag & LPFC_VMID_TIMER_ENBLD)) {
>>> + mod_timer(&vport->phba->inactive_vmid_poll,
>>> + jiffies +
>>> + msecs_to_jiffies(1000 * LPFC_VMID_TIMER));
>>> + vport->phba->pport->vmid_flag |= LPFC_VMID_TIMER_ENBLD;
>>> + }
>>> + }
>>> + return rc;
>>> +}
>>> +
>>> +/*
>>> + * lpfc_is_command_vm_io - get the uuid from blk cgroup
>>> + * @cmd:Pointer to scsi_cmnd data structure
>>> + * Returns uuid if present if not null
>>> + */
>>> +static char *lpfc_is_command_vm_io(struct scsi_cmnd *cmd)
>>> +{
>>> + char *uuid = NULL;
>>> +
>>> + if (cmd->request) {
>>> + if (cmd->request->bio)
>>> + uuid = blkcg_get_fc_appid(cmd->request->bio);
>>> + }
>>> + return uuid;
>>> +}
>>> +
>>> /**
>>> * lpfc_queuecommand - scsi_host_template queuecommand entry point
>>> * @shost: kernel scsi host pointer.
>>> @@ -5288,6 +5437,7 @@ lpfc_queuecommand(struct Scsi_Host *shost,
>>> struct scsi_cmnd *cmnd)
>>> int err, idx;
>>> #ifdef CONFIG_SCSI_LPFC_DEBUG_FS
>>> uint64_t start = 0L;
>>> + u8 *uuid = NULL;
>>> if (phba->ktime_on)
>>> start = ktime_get_ns();
>>> @@ -5415,6 +5565,25 @@ lpfc_queuecommand(struct Scsi_Host *shost,
>>> struct scsi_cmnd *cmnd)
>>> }
>>> + /* check the necessary and sufficient condition to support VMID */
>>> + if (lpfc_is_vmid_enabled(phba) &&
>>> + (ndlp->vmid_support ||
>>> + phba->pport->vmid_priority_tagging ==
>>> + LPFC_VMID_PRIO_TAG_ALL_TARGETS)) {
>>> + /* is the IO generated by a VM, get the associated virtual */
>>> + /* entity id */
>>> + uuid = lpfc_is_command_vm_io(cmnd);
>>> +
>>> + if (uuid) {
>>> + err = lpfc_vmid_get_appid(vport, uuid, cmnd,
>>> + (union lpfc_vmid_io_tag *)
>>> + &lpfc_cmd->cur_iocbq.vmid_tag);
>>> + if (!err)
>>> + lpfc_cmd->cur_iocbq.iocb_flag |= LPFC_IO_VMID;
>>> + }
>>> + }
>>> +
>>> + atomic_inc(&ndlp->cmd_pending);
>>> #ifdef CONFIG_SCSI_LPFC_DEBUG_FS
>>> if (unlikely(phba->hdwqstat_on & LPFC_CHECK_SCSI_IO))
>>> this_cpu_inc(phba->sli4_hba.c_stat->xmt_io);
>>>
>> And that's the bit which I don't particular like.
>>
>> Essentially we'll have to inject additional ELS commands _on each I/O_
>> to get a valid VMID.
>> Where there are _so_ many things which might get wrong, causing an I/O
>> stall.
>
> I don't follow you - yes ELS's are injected when there isn't an entry
> for the VM, but once there is, there isn't further ELS's. That is the
> cost. as we don't know what uuid's I/O will be sent to before hand, so
> it has to be siphoned off during the I/O flow. I/O's can be sent
> non-tagged while the ELS's are completing (and there aren't multiple
> sets of ELS's as long as it's the same uuid), which is fine.
>
> so I disagree with "_on each I/O_".
>
Yeah, that was an exaggeration.
Not on each I/O; I should've said 'on each unregistered I/O'.
This really is my unhappiness with the entire VMID specification,
where you can time out VMIDs after a certain period, and hence never
be sure if any particular I/O really _is_ registered.
>> I would have vastly preferred if we could _avoid_ having to do
>> additional ELS commands for VMID registration in the I/O path
>> (ie only allow for I/O with a valid VMID), and reject the I/O
>> otherwise until VMID registration is complete.
>>
>> IE return 'BUSY' (or even a command retry?) when no valid VMID for
>> this particular I/O is found, register the VMID (preferably in another
>> thread), and restart the queue once the VMID is registered.
>
> Why does it bother you with the I/O path ? It's actually happening in
> parallel with no real relation between the two.
>
> I seriously disagree with reject if no vmid tag. Why? what do you gain
> ? This actually introduces more disruption than the parallel flow with
> the ELSs. Also, after link bounce, where all VMID's have to be done,
> it adds a stall window after link up right when things are trying to
> resume after rports rejoin. Why add the i/o rebouncing ? There no real
> benefit. Issuing a few untagged I/O doesn't hurt.
>
That indeed is a valid point. I retract my objection.
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
prev parent reply other threads:[~2021-04-12 5:27 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-06 23:06 [PATCH v9 00/13] blkcg:Support to track FC storage blk io traffic Muneendra
2021-04-06 23:06 ` [PATCH v9 01/13] cgroup: Added cgroup_get_from_id Muneendra
2021-04-08 8:26 ` Hannes Reinecke
2021-04-06 23:06 ` [PATCH v9 02/13] blkcg: Added a app identifier support for blkcg Muneendra
2021-04-08 8:26 ` Hannes Reinecke
2021-04-06 23:06 ` [PATCH v9 03/13] nvme: Added a newsysfs attribute appid_store Muneendra
2021-04-18 15:32 ` Benjamin Block
2021-04-20 6:54 ` Muneendra Kumar M
2021-04-20 11:09 ` Benjamin Block
2021-04-22 23:29 ` James Smart
2021-04-23 10:14 ` Benjamin Block
2021-04-06 23:06 ` [PATCH v9 04/13] lpfc: vmid: Add the datastructure for supporting VMID in lpfc Muneendra
2021-04-08 8:28 ` Hannes Reinecke
2021-04-06 23:06 ` [PATCH v9 05/13] lpfc: vmid: VMID params initialization Muneendra
2021-04-08 8:29 ` Hannes Reinecke
2021-04-06 23:06 ` [PATCH v9 06/13] lpfc: vmid: Add support for vmid in mailbox command, does vmid resource allocation and vmid cleanup Muneendra
2021-04-08 8:32 ` Hannes Reinecke
2021-04-06 23:06 ` [PATCH v9 07/13] lpfc: vmid: Implements ELS commands for appid patch Muneendra
2021-04-08 8:34 ` Hannes Reinecke
2021-04-20 12:38 ` Benjamin Block
2021-04-21 22:55 ` James Smart
2021-04-22 9:28 ` Benjamin Block
2021-04-06 23:06 ` [PATCH v9 08/13] lpfc: vmid: Functions to manage vmids Muneendra
2021-04-08 8:35 ` Hannes Reinecke
2021-04-06 23:06 ` [PATCH v9 09/13] lpfc: vmid: Implements CT commands for appid Muneendra
2021-04-08 8:37 ` Hannes Reinecke
2021-04-06 23:06 ` [PATCH v9 10/13] lpfc: vmid: Appends the vmid in the wqe before sending Muneendra
2021-04-06 23:06 ` [PATCH v9 11/13] lpfc: vmid: Timeout implementation for vmid Muneendra
2021-04-08 8:38 ` Hannes Reinecke
2021-04-06 23:06 ` [PATCH v9 12/13] lpfc: vmid: Adding qfpa and vmid timeout check in worker thread Muneendra
2021-04-06 23:06 ` [PATCH v9 13/13] lpfc: vmid: Introducing vmid in io path Muneendra
2021-04-08 8:46 ` Hannes Reinecke
2021-04-10 15:00 ` James Smart
2021-04-12 5:27 ` Hannes Reinecke [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=edbcc321-fba1-e908-7932-ecb6b70ffc50@suse.de \
--to=hare@suse.de \
--cc=emilne@redhat.com \
--cc=gaurav.srivastava@broadcom.com \
--cc=jsmart2021@gmail.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=linux-scsi@vger.kernel.org \
--cc=mkumar@redhat.com \
--cc=muneendra.kumar@broadcom.com \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).