[MODERATED] Re: [PATCH v5 09/11] TAAv5 9

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Oct 10, 2019 at 02:31:51PM -0700, speck for Pawan Gupta wrote:
> On Mon, Oct 07, 2019 at 11:01:56PM -0700, speck for Pawan Gupta wrote:
> > > > +static DEFINE_MUTEX(tsx_mutex);
> > > 
> > > I think I asked this before, but in looking at the code I still can't
> > > figure it out.  What exactly is this protecting?
> > > 
> > > It looks like you want to keep only one "writer" out of the sysfs store
> > > function at a time, but:
> > > 
> > > > +ssize_t hw_tx_mem_store(struct device *dev, struct device_attribute *attr,
> > > > +			const char *buf, size_t count)
> > > > +{
> > > > +	enum tsx_ctrl_states requested_state;
> > > > +	ssize_t ret;
> > > > +	bool val;
> > > > +
> > > > +	ret = kstrtobool(buf, &val);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	mutex_lock(&tsx_mutex);
> > > > +
> > > > +	if (val) {
> > > > +		tsx_user_cmd = TSX_USER_CMD_ON;
> > > > +		requested_state = TSX_CTRL_ENABLE;
> > > > +	} else {
> > > > +		tsx_user_cmd = TSX_USER_CMD_OFF;
> > > > +		requested_state = TSX_CTRL_DISABLE;
> > > > +	}
> > > > +
> > > > +	/* Current state is same as the reqested state, do nothing */
> > > > +	if (tsx_ctrl_state == requested_state)
> > > > +		goto exit;
> > > > +
> > > > +	tsx_ctrl_state = requested_state;
> > > > +
> > > > +	tsx_update_on_each_cpu(val);
> > > > +exit:
> > > > +	mutex_unlock(&tsx_mutex);
> > > 
> > > What I think you want to do is just protect the tsx_update_on_each_cpu()
> > > function, right?
> > 
> > Also I believe below two operations needs to be under a lock. Without
> > the lock if there are two writers and one is preempted in between these
> > operations there is a possibility that tsx_ctrl_state and TSX hardware
> > state could go out of sync.
> > 
> > 	tsx_ctrl_state = requested_state;
> > 
> > 	// 1st writer gets preempted here
> > 	// 2nd writer flips tsx_ctrl_state and writes to the MSR.
> > 	// 1st writer wakes up and only writes to the MSR
> > 
> > 	tsx_update_on_each_cpu(val);
> > 	// tsx_ctrl_state and hardware state would be different here.
> > 
> > Chances of this happening is rare but still a possibility. The lock
> > would prevent such a condition.
> 
> Greg, is it not a possible scenario for tsx_ctrl_state and MSR write to
> be under the lock.

I'm sorry, but I do not understand what you are trying to state here.

Look at the code, and the lock, and determine what it really is doing
and document that please.  Right now I can't determine it and it looks
pointless to me, so I must be missing something.

thanks,

greg k-h