Hi Brian,
Thanks for reviewing. Comments inline below.
-matt
On Jul 10, 2015, at 2:49 PM, Brian King wrote:
> On 06/19/2015 05:37 PM, Matthew R. Ochs wrote:
>>
>> cfg->init_state = INIT_STATE_NONE;
>> cfg->dev = pdev;
>> + cfg->last_lun_index[0] = 0;
>> + cfg->last_lun_index[1] = 0;
>> cfg->dev_id = (struct pci_device_id *)dev_id;
>> cfg->mcctx = NULL;
>> cfg->err_recovery_active = 0;
>> + cfg->num_user_contexts = 0;
>
> No need to set these to zero, since they get allocated via scsi_host_alloc,
> which zeroes the memory for you.
Agreed. We've actually already fixed this, you'll see it in v2.
>> +/**
>> + * marshall_det_to_rele() - translate detach to release structure
>> + * @detach: Destination structure for the translate/copy.
>> + * @rele: Source structure from which to translate/copy.
>> + */
>> +static void marshall_det_to_rele(struct dk_cxlflash_detach *detach,
>> + struct dk_cxlflash_release *release)
>> +{
>> + release->hdr = detach->hdr;
>> + release->context_id = detach->context_id;
>
> Function name could be better. Marshal should have only one 'l' in this usage...
Sure, will fix.
>> + if (wwid)
>> + list_for_each_entry_safe(lun_info, temp, &global.luns, list) {
>> + if (!memcmp(lun_info->wwid, wwid,
>> + DK_CXLFLASH_MANAGE_LUN_WWID_LEN))
>> + return lun_info;
>> + }
>
> You probably want to be grabbing global.slock hre, right? list_for_each_entry_safe doesn't protect
> you from other threads deleting things off the list, it only allows you to delete
> entries off the list in your loop and continue to iterate.
Yes, will correct.
>> +
>> + /* Store off lun in unpacked, AFU-friendly format */
>> + lun_info->lun_id = lun_to_lunid(sdev->lun);
>> + lun_info->lun_index = cfg->last_lun_index[sdev->channel];
>> +
>> + writeq_be(lun_info->lun_id,
>> + &afu->afu_map->global.fc_port[sdev->channel]
>> + [cfg->last_lun_index[sdev->channel]++]);
>
> Do you need a slave_destroy that undoes this and makes the AFU forget
> about this LUN? I'm thinking you probably want to also destroy any
> user contexts to this device in slave_destroy as well. Otherwise
> if you have a superpipe open to a scsi_device and someone deletes that
> scsi_device through sysfs, you will have an open superpipe to that
> device still and no way to tear it down since you don't have a
> way to issue the detach ioctl any longer.
In v2, you'll see that we have remove any need for the slave_* routines.
>> + pid_t pid = current->tgid, ctxpid = 0;
>> + ulong flags = 0;
>> +
>> + if (unlikely(ctx_ctrl & CTX_CTRL_CLONE))
>
> I'm seeing what is a bit of an overuse of likely / unlikely. Basically, IMHO,
> the only place where you should be using these compiler hints is in the performance
> path and even then, only when the odds are pretty high that you will go down
> the likely path.
We've actually revised the likely/unlikely since this patch. You'll see it toned down in v2.
>>
>> + if (likely(lun_info)) {
>> + list_for_each_entry(lun_access, &ctx_info->luns, list)
>> + if (lun_access->lun_info == lun_info) {
>> + found = true;
>> + break;
>> + }
>
> Do we need some locking here?
Yes, will investigate adding a per-context lock.
>> +static int read_cap16(struct afu *afu, struct lun_info *lun_info, u32 port_sel)
>> +{
>> + struct afu_cmd *cmd = NULL;
>> + int rc = 0;
>> +
>
> Suggest using scsi_execute instead. That way it goes up through the block layer
> via the normal execution path, should result in less code, and then you also get
> error recovery for free. You can then get rid of the nasty looping on cxlflash_check_status.
Done.
>> + cmd = cxlflash_cmd_checkout(afu);
>> + if (unlikely(!cmd)) {
>> + pr_err("%s: could not get a free command\n", __func__);
>> + return -1;
>> + }
>> +
>> + cmd->rcb.req_flags = (SISL_REQ_FLAGS_PORT_LUN_ID |
>> + SISL_REQ_FLAGS_SUP_UNDERRUN |
>> + SISL_REQ_FLAGS_HOST_READ);
>> +
>> + cmd->rcb.port_sel = port_sel;
>> + cmd->rcb.lun_id = lun_info->lun_id;
>> + cmd->rcb.data_len = CMD_BUFSIZE;
>> + cmd->rcb.data_ea = (u64) cmd->buf;
>> + cmd->rcb.timeout = MC_DISCOVERY_TIMEOUT;
>> +
>> + cmd->rcb.cdb[0] = 0x9E; /* read cap(16) */
>
> Use SERVICE_ACTION_IN_16 here
Okay.
>> + cmd->rcb.cdb[1] = 0x10; /* service action */
>
> Use SAI_READ_CAPACITY_16 here
You got it.
>> + * rhte_checkin() - releases a resource handle table entry
>> + * @ctx_info: Context owning the resource handle.
>> + * @rht_entry: RHTE to release.
>> + */
>> +void rhte_checkin(struct ctx_info *ctx_info,
>> + struct sisl_rht_entry *rht_entry)
>> +{
>> + rht_entry->nmask = 0;
>> + rht_entry->fp = 0;
>> + ctx_info->rht_out--;
>> + ctx_info->rht_lun[rht_entry - ctx_info->rht_start] = NULL;
>
> Do you need some locking in both the checkout and checkin path?
Likely, will investigate this.
>>
>> + /*
>> + * Clear the Format 1 RHT entry for direct access
>> + * (physical LUN) using the synchronization sequence
>> + * defined in the SISLite specification.
>> + */
>> + rht_entry_f1 = (struct sisl_rht_entry_f1 *)rht_entry;
>> +
>> + rht_entry_f1->valid = 0;
>> + smp_wmb(); /* Make revocation of RHT entry visible */
>
> I think what you actually want to use here is dma_wmb() rather than smp_wmb(),
> assuming you are trying to ensure these writes occur in order with respect
> to the AFU. If you were to build the kernel as a UP kernel, rather than an SMP
> kernel, all these smp_wmb calls would turn into mere compiler barriers, which is
> not what you want, I think... Suggest auditing the entire patch as there are
> likely other barriers you may want to change as well.
What we're looking for here is a lightweight sync. Looking at the PPC barrier.h, I see
that you are correct, we need to specify dma_wmb() instead to ensure we use the
LWSYNC instead of a compiler barrier() when smp_wmb is not defined.. Agree with
the audit idea.
>> + /* Free the context; note that rht_lun was allocated at same time */
>> + kfree(ctx_info);
>> + cfg->num_user_contexts--;
>
> What guarantees atomicity of this increment? I don't see any locking around the callers...
Per Mikey Neuling's comments, we have already made this atomic.
>> + /* Take our LUN out of context, free the node */
>> + list_for_each_entry_safe(lun_access, t, &ctx_info->luns, list)
>> + if (lun_access->lun_info == lun_info) {
>> + list_del(&lun_access->list);
>> + kfree(lun_access);
>> + lun_access = NULL;
>> + break;
>> + }
>
> Do you need to do some locking around this? Can others be reading / writing
> to / from this list concurrently?
Reading/writing would be via any of the ioctl threads when determining access
to a context. We'll likely cover this via a per-context lock.
>> +
>> + /* Tear down context following last LUN cleanup */
>> + if (list_empty(&ctx_info->luns)) {
>> + spin_lock_irqsave(&cfg->ctx_tbl_slock, flags);
>> + cfg->ctx_tbl[ctxid] = NULL;
>> + spin_unlock_irqrestore(&cfg->ctx_tbl_slock, flags);
>> +
>> + while (atomic_read(&ctx_info->nrefs) > 1) {
>> + pr_debug("%s: waiting on threads... (%d)\n",
>> + __func__, atomic_read(&ctx_info->nrefs));
>> + cpu_relax();
>
> Hmm. Would an msleep with an overall timeout be more appropriate here? It might
> actually be cleaner just to use a kref in the ctx_info struct to manage your
> reference count and then use the release function to do that cleanup,
> then you could simplify all this and eliminate the wait.
In v2, you'll see that we've already move to a sleep instead of a busy-wait here. Will
need to investigate what moving to a kref would look like.
>> +int cxlflash_mark_contexts_error(struct cxlflash_cfg *cfg)
>> +{
>> + int i, rc = 0;
>> + ulong lock_flags;
>> + struct ctx_info *ctx_info = NULL;
>> +
>> + spin_lock_irqsave(&cfg->ctx_tbl_slock, lock_flags);
>> +
>> + for (i = 0; i < MAX_CONTEXT; i++) {
>> + ctx_info = cfg->ctx_tbl[i];
>> +
>> + if (ctx_info) {
>> + cfg->ctx_tbl[i] = NULL;
>> + list_add(&ctx_info->list, &cfg->ctx_err_recovery);
>> + ctx_info->err_recovery_active = true;
>> + unmap_context(ctx_info);
>> + }
>> + }
>> +
>> + spin_unlock_irqrestore(&cfg->ctx_tbl_slock, lock_flags);
>> +
>> + return rc;
>> +}
>
> This function doesn't appear to be called anywhere. Not even in the follow on patch...
You're correct, this might not have been called in the first patch, but is used in v2.
>> +
>> + /* On first attach set fileops */
>> + if (cfg->num_user_contexts == 0)
>> + cfg->cxl_fops = cxlflash_cxl_fops;
>
> I'm not seeing any synchronization or locking here. How are you managing a hundred different users
> hitting your ioctl path at the same time? You are iterating through lists below with no locks
> held, but it seems to me like that list could be changing on you. Am I missing something here?
This line in particular is now an atomic. We've also added a state check to block ioctls when needed.
>> + ctxid = cxl_process_element(ctx);
>> + if ((ctxid > MAX_CONTEXT) || (ctxid < 0)) {
>
> Too many parentheses
Fixed.
>> + reg = readq_be(&afu->ctrl_map->mbox_r); /* Try MMIO */
>> + /* MMIO returning 0xff, need to reset */
>> + if (reg == -1) {
>> + pr_info("%s: afu=%p reason 0x%llx\n",
>> + __func__, afu, recover->reason);
>> + cxlflash_afu_reset(cfg);
>> + } else {
>> + pr_debug("%s: reason 0x%llx MMIO working, no reset performed\n",
>> + __func__, recover->reason);
>> + rc = -EINVAL;
>
> This seems a little strange to me. The user asked you to recover the AFU, the AFU looks
> fine to you, so you do nothing, yet you fail the request to recover the AFU. What about
> multiple users issuing this IOCTL at the same time? Seems like we might want to just
> return success rather than make the user second guess what they did wrong in building
> the ioctl.
Correct, we've reworked this routine for v2. You'll see behavior similar to as you've described.
>> + case MODE_VIRTUAL:
>> + last_lba = (((rht_entry->lxt_cnt * MC_CHUNK_SIZE *
>> + lun_info->blk_len) / CXLFLASH_BLOCK_SIZE) - 1);
>> + break;
>
> Should this go in the second patch?
Yes, this should have been in the second patch.
>> +struct dk_cxlflash_uvirtual {
>> + struct dk_cxlflash_hdr hdr; /* Common fields */
>> + __u64 context_id; /* Context to own virtual resources */
>> + __u64 lun_size; /* Requested size, in 4K blocks */
>> + __u64 rsrc_handle; /* Returned resource handle */
>> + __u64 last_lba; /* Returned last LBA of LUN */
>> +};
>
> This isn't used in this patch, so should really be in the second patch.
Yes, this technically should be in the second patch.
>> +
>> +struct dk_cxlflash_release {
>> + struct dk_cxlflash_hdr hdr; /* Common fields */
>> + __u64 context_id; /* Context owning resources */
>> + __u64 rsrc_handle; /* Resource handle to release */
>> +};
>> +
>> +struct dk_cxlflash_resize {
>> + struct dk_cxlflash_hdr hdr; /* Common fields */
>> + __u64 context_id; /* Context owning resources */
>> + __u64 rsrc_handle; /* Resource handle of LUN to resize */
>> + __u64 req_size; /* New requested size, in 4K blocks */
>> + __u64 last_lba; /* Returned last LBA of LUN */
>> +};
>
> Looks like the same is true here. Ideally all the virtual LUN definitions, function
> prototypes, etc, would be in the second patch.
Agreed.
>> +#define DK_CXLFLASH_ATTACH CXL_IOW(0x80, dk_cxlflash_attach)
>
> It would probably be good to add a comment to include/uapi/misc/cxl.h to indicate
> that cxlflash is reserving this ioctl range so you don't conflict with other
> cxl users.
Sounds reasonable. Will work with the maintainers of that file to do this.
>> +#define DK_CXLFLASH_USER_DIRECT CXL_IOW(0x81, dk_cxlflash_udirect)
>> +#define DK_CXLFLASH_USER_VIRTUAL CXL_IOW(0x82, dk_cxlflash_uvirtual)
>> +#define DK_CXLFLASH_VLUN_RESIZE CXL_IOW(0x83, dk_cxlflash_resize)
>> +#define DK_CXLFLASH_RELEASE CXL_IOW(0x84, dk_cxlflash_release)
>> +#define DK_CXLFLASH_DETACH CXL_IOW(0x85, dk_cxlflash_detach)
>> +#define DK_CXLFLASH_VERIFY CXL_IOW(0x86, dk_cxlflash_verify)
>> +#define DK_CXLFLASH_CLONE CXL_IOW(0x87, dk_cxlflash_clone)
>> +#define DK_CXLFLASH_RECOVER_AFU CXL_IOW(0x88, dk_cxlflash_recover_afu)
>> +#define DK_CXLFLASH_MANAGE_LUN CXL_IOW(0x89, dk_cxlflash_manage_lun)
>> +
>> +#endif /* ifndef _CXLFLASH_IOCTL_H */
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
