> On Aug 11, 2015, at 10:54 PM, Michael Neuling <mi...@neuling.org> wrote:
>
>
>>>> + pr_debug("%s: Wait for user context to quiesce...\n", __func__);
>>>> + wake_up_all(&cfg->limbo_waitq);
>>>> + ssleep(1);
>>>
>>> Why 1 sec and why in a loop? Can’t you poll/wait for completion somewhere?
>>
>> This routine is called when the device is being removed and we need to stall
>> until
>> the user contexts are made aware of removal [by marking them in error] and
>> have
>> completely gone away [there are no longer any contexts in our table or list].
>>
>> The 1 second sleep is simply to give the users time to cleanup once they see
>> the notification. We can make it a larger/smaller value or remove it
>> entirely, but
>> I felt that since this is not a hot path there was no reason to perform a
>> busy-wait
>> style of loop here and yield to someone else.
>>
>> As for the completion/poll, I did consider that but couldn’t come up with a
>> clean
>> way of implementing given how we designed the notification/cleanup mechanism
>> (really just a failed recovery). We can certainly look at doing that as an
>> enhancement in the future.
>
> Does the API allow this flexibility in the future?
Yes, I believe it is flexible enough where it could allow for this.
>>
>>>> + rhte_checkin(ctxi, rhte);
>>>> + cxlflash_lun_detach(gli);
>>>> +
>>>> +out:
>>>> + if (unlock_ctx)
>>>> + mutex_unlock(&ctxi->mutex);
>>>
>>> Where is the matching lock for this?
>>
>> One of the main design points of our context serialization strategy is that
>> any context returned from get_context() has been fully validated, will not
>> be removed from under you, and _is holding the context mutex_. Thus
>> for each of these mutex_unlock(ctxi) you see at the bottom of external
>> entry points, the mutex was obtained in get_context().
>
> Should we have something like put_context() that does this? So there is
> matching calls to get/put_context
Yes, this is a really good idea (I wish I’d thought of it ;)) that will make for
better code. Look for it in v5!
>
>>
>>>> + char *tmp = NULL;
>>>> + size_t size;
>>>> + struct afu *afu = cfg->afu;
>>>> + struct ctx_info *ctxi = NULL;
>>>> + struct sisl_rht_entry *rhte;
>>>> +
>>>> + size = (MAX_RHT_PER_CONTEXT * sizeof(*ctxi->rht_lun));
>>>> + size += sizeof(*ctxi);
>>>> +
>>>> + tmp = kzalloc(size, GFP_KERNEL);
>>>
>>> Just do two allocs. One for ctxi and one for rht_lun. This is overly
>>> complicated.
>>
>> I disagree that it’s overly complicated. The intention here was to get this
>> stuff together to avoid multiple function calls and possibly help out
>> perf-wise
>> via locality. That said, I’m not opposed to performing multiple allocations.
>
> I'm not sure I buy it's a performance issue on create_context(). How
> often are you calling that? Doesn't that do a lot of things other than
> just this?
>
> Anyway if you want to do that, that's ok I guess, but you need to
> document why and what you’re doing.
We’ve already made the change to perform multiple allocations and
are ‘okay’ with that.
>>>> + hdr = (struct dk_cxlflash_hdr *)&buf;
>>>> + if (hdr->version != 0) {
>>>> + pr_err("%s: Version %u not supported for %s\n",
>>>> + __func__, hdr->version, decode_ioctl(cmd));
>>>> + rc = -EINVAL;
>>>> + goto cxlflash_ioctl_exit;
>>>> + }
>>>
>>> Do you advertise this version anywhere? Users just have to call it and
>>> fail?
>>
>> We don’t. We can add a version define to the exported ioctl header.
>
> It needs to be exported dynamically by the kernel. Not the headers.
> Think new software on old kernel and visa versa.
Okay, will look into this.
>>>> +#define DK_CXLFLASH_ATTACH CXL_IOWR(0x80,
>>>> dk_cxlflash_attach)
>>>> +#define DK_CXLFLASH_USER_DIRECT CXL_IOWR(0x81,
>>>> dk_cxlflash_udirect)
>>>> +#define DK_CXLFLASH_RELEASE CXL_IOWR(0x84,
>>>> dk_cxlflash_release)
>>>> +#define DK_CXLFLASH_DETACH CXL_IOWR(0x85,
>>>> dk_cxlflash_detach)
>>>> +#define DK_CXLFLASH_VERIFY CXL_IOWR(0x86,
>>>> dk_cxlflash_verify)
>>>> +#define DK_CXLFLASH_RECOVER_AFU CXL_IOWR(0x88,
>>>> dk_cxlflash_recover_afu)
>>>> +#define DK_CXLFLASH_MANAGE_LUN CXL_IOWR(0x89,
>>>> dk_cxlflash_manage_lun)
>>>
>>> I'm not sure I'd leave these sparse. What happens if the vlun patches don't
>>> get in?
>>
>> I do agree with you. I had wanted to keep them like this as their ordering
>> matched
>> closer with how they are expected to be used. That said, I’m okay with moving
>> them around to avoid the sparseness.
>
> Yeah, plus it breaks your table in the ioctl
Right. We had originally thought about adding reserved placeholders to
avoid the breakage in the ioctl table but have gone ahead and just reordered.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
