Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
From: James Smart <jsmart2021@gmail.com>
To: Hannes Reinecke <hare@suse.de>,
	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: Sat, 10 Apr 2021 08:00:27 -0700
Message-ID: <201c6604-8e9d-bfcb-f39e-be507f8b02d6@gmail.com> (raw)
In-Reply-To: <91b0c309-6908-8fd9-ac60-a8572500c3ed@suse.de>

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

> 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 way we have a clear separation, and the I/O path will always work 
> with valid VMIDs.
> 
> Hmm?
> 
> Cheers,
> 
> Hannes

-- james


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

  reply index

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 [this message]
2021-04-12  5:27       ` Hannes Reinecke

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=201c6604-8e9d-bfcb-f39e-be507f8b02d6@gmail.com \
    --to=jsmart2021@gmail.com \
    --cc=emilne@redhat.com \
    --cc=gaurav.srivastava@broadcom.com \
    --cc=hare@suse.de \
    --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

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git