On Mon, Mar 30, 2020 at 07:49:00PM +0000, Michael Kelley wrote:
> From: Andrea Parri <parri.andrea@xxxxxxxxx> Sent: Monday, March 30, 2020 11:55 AM
> >
> > > > @@ -1721,6 +1721,10 @@ static ssize_t target_cpu_store(struct vmbus_channel
> > *channel,
> > > > * in on a CPU that is different from the channel target_cpu value.
> > > > */
> > > >
> > > > + if (channel->change_target_cpu_callback)
> > > > + (*channel->change_target_cpu_callback)(channel,
> > > > + channel->target_cpu, target_cpu);
> > > > +
> > > > channel->target_cpu = target_cpu;
> > > > channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
> > > > channel->numa_node = cpu_to_node(target_cpu);
> > >
> > > I think there's an ordering problem here. The change_target_cpu_callback
> > > will allow storvsc to flush the cache that it is keeping, but there's a window
> > > after the storvsc callback releases the spin lock and before this function
> > > changes channel->target_cpu to the new value. In that window, the cache
> > > could get refilled based on the old value of channel->target_cpu, which is
> > > exactly what we don't want. Generally with caches, you have to set the new
> > > value first, then flush the cache, and I think that works in this case. The
> > > callback function doesn't depend on the value of channel->target_cpu,
> > > and any cache filling that might happen after channel->target_cpu is set
> > > to the new value but before the callback function runs is OK. But please
> > > double-check my thinking. :-)
> >
> > Sorry, I don't see the problem. AFAICT, the "cache" gets refilled based
> > on the values of alloced_cpus and on the current state of the cache but
> > not based on the value of channel->target_cpu. The callback invocation
> > uses the value of the "old" target_cpu; I think I ended up placing the
> > callback call where it is for not having to introduce a local variable
> > "old_cpu". ;-)
> >
>
> You are right. My comment is bogus.
>
> >
> > > > @@ -621,6 +621,63 @@ static inline struct storvsc_device *get_in_stor_device(
> > > >
> > > > }
> > > >
> > > > +void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old, u32 new)
> > > > +{
> > > > + struct storvsc_device *stor_device;
> > > > + struct vmbus_channel *cur_chn;
> > > > + bool old_is_alloced = false;
> > > > + struct hv_device *device;
> > > > + unsigned long flags;
> > > > + int cpu;
> > > > +
> > > > + device = channel->primary_channel ?
> > > > + channel->primary_channel->device_obj
> > > > + : channel->device_obj;
> > > > + stor_device = get_out_stor_device(device);
> > > > + if (!stor_device)
> > > > + return;
> > > > +
> > > > + /* See storvsc_do_io() -> get_og_chn(). */
> > > > + spin_lock_irqsave(&device->channel->lock, flags);
> > > > +
> > > > + /*
> > > > + * Determines if the storvsc device has other channels assigned to
> > > > + * the "old" CPU to update the alloced_cpus mask and the stor_chns
> > > > + * array.
> > > > + */
> > > > + if (device->channel != channel && device->channel->target_cpu == old) {
> > > > + cur_chn = device->channel;
> > > > + old_is_alloced = true;
> > > > + goto old_is_alloced;
> > > > + }
> > > > + list_for_each_entry(cur_chn, &device->channel->sc_list, sc_list) {
> > > > + if (cur_chn == channel)
> > > > + continue;
> > > > + if (cur_chn->target_cpu == old) {
> > > > + old_is_alloced = true;
> > > > + goto old_is_alloced;
> > > > + }
> > > > + }
> > > > +
> > > > +old_is_alloced:
> > > > + if (old_is_alloced)
> > > > + WRITE_ONCE(stor_device->stor_chns[old], cur_chn);
> > > > + else
> > > > + cpumask_clear_cpu(old, &stor_device->alloced_cpus);
> > >
> > > I think target_cpu_store() can get called in parallel on multiple CPUs for different
> > > channels on the same storvsc device, but multiple changes to a single channel are
> > > serialized by higher levels of sysfs. So this function could run after multiple
> > > channels have been changed, in which case there's not just a single "old" value,
> > > and the above algorithm might not work, especially if channel->target_cpu is
> > > updated before calling this function per my earlier comment. I can see a
> > > couple of possible ways to deal with this. One is to put the update of
> > > channel->target_cpu in this function, within the spin lock boundaries so
> > > that the cache flush and target_cpu update are atomic. Another idea is to
> > > process multiple changes in this function, by building a temp copy of
> > > alloced_cpus by walking the channel list, use XOR to create a cpumask
> > > with changes, and then process all the changes in a loop instead of
> > > just handling a single change as with the current code at the old_is_alloced
> > > label. But I haven't completely thought through this idea.
> >
> > Same here: the invocations of target_cpu_store() are serialized on the
> > per-connection channel_mutex...
>
> Agreed. My comment is not valid.
>
> >
> >
> > > > @@ -1268,8 +1330,10 @@ static struct vmbus_channel *get_og_chn(struct
> > storvsc_device
> > > > *stor_device,
> > > > if (cpumask_test_cpu(tgt_cpu, node_mask))
> > > > num_channels++;
> > > > }
> > > > - if (num_channels == 0)
> > > > + if (num_channels == 0) {
> > > > + stor_device->stor_chns[q_num] = stor_device->device->channel;
> > >
> > > Is the above added line just fixing a bug in the existing code? I'm not seeing how
> > > it would derive from the other changes in this patch.
> >
> > It was rather intended as an optimization: Each time I/O for a device
> > is initiated on a CPU that have "num_channels == 0" channel, the current
> > code ends up calling get_og_chn() (in the attempt to fill the cache) and
> > returns the device's primary channel. In the current code, the cost of
> > this operations is basically the cost of parsing alloced_cpus, but with
> > the changes introduced here this also involves acquiring (and releasing)
> > the primary channel's lock. I should probably put my hands forward and
> > say that I haven't observed any measurable effects due this addition in
> > my experiments; OTOH, caching the returned/"found" value made sense...
>
> OK. That's what I thought. The existing code does not produce an incorrect
> result, but the cache isn't working as intended. This fixes it.
>
> >
> >
> > > > @@ -1324,7 +1390,10 @@ static int storvsc_do_io(struct hv_device *device,
> > > > continue;
> > > > if (tgt_cpu == q_num)
> > > > continue;
> > > > - channel = stor_device->stor_chns[tgt_cpu];
> > > > + channel = READ_ONCE(
> > > > + stor_device->stor_chns[tgt_cpu]);
> > > > + if (channel == NULL)
> > > > + continue;
> > >
> > > The channel == NULL case is new because a cache flush could be happening
> > > in parallel on another CPU. I'm wondering about the tradeoffs of
> > > continuing in the loop (as you have coded in this patch) vs. a "goto" back to
> > > the top level "if" statement. With the "continue" you might finish the
> > > loop without finding any matches, and fall through to the next approach.
> > > But it's only a single I/O operation, and if it comes up with a less than
> > > optimal channel choice, it's no big deal. So I guess it's really a wash.
> >
> > Yes, I considered both approaches; they both "worked" here. I was a
> > bit concerned about the number of "possible" gotos (again, mainly a
> > theoretical issue, since I can imagine that the cash flushes will be
> > relatively "rare" events in most cases and, in any case, they happen
> > to be serialized); the "continue" looked like a suitable and simpler
> > approach/compromise, at least for the time being.
>
> Yes, I'm OK with your patch "as is". I was just thinking about the
> alternative, and evidently you did too.
Thank you for the confirmation. I'm wondering: could take this as a
Reviewed-by: for this patch? ;-)
Thanks,
Andrea
>
> >
> >
> > >
> > > > if (hv_get_avail_to_write_percent(
> > > > &channel->outbound)
> > > > > ring_avail_percent_lowater) {
> > > > @@ -1350,7 +1419,10 @@ static int storvsc_do_io(struct hv_device *device,
> > > > for_each_cpu(tgt_cpu, &stor_device->alloced_cpus) {
> > > > if (cpumask_test_cpu(tgt_cpu, node_mask))
> > > > continue;
> > > > - channel = stor_device->stor_chns[tgt_cpu];
> > > > + channel = READ_ONCE(
> > > > + stor_device->stor_chns[tgt_cpu]);
> > > > + if (channel == NULL)
> > > > + continue;
> > >
> > > Same comment here.
> >
> > Similarly here.
>
> Agreed.
>
> >
> > Thoughts?
> >
> > Thanks,
> > Andrea
