| Message ID | 1577576793-113222-2-git-send-email-haiyangz@microsoft.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add vmbus dev_num and enable netvsc async probing | expand |
On Sat, 28 Dec 2019 15:46:31 -0800 Haiyang Zhang <haiyangz@microsoft.com> wrote: > + > +next: > + found = false; > + > + list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) { > + if (i == channel->dev_num && > + guid_equal(&channel->offermsg.offer.if_type, > + &newchannel->offermsg.offer.if_type)) { > + found = true; > + break; > + } > + } > + > + if (found) { > + i++; > + goto next; > + } > + Overall, keeping track of dev_num is a good solution. I prefer not having a loop coded with goto's. Why not a nested loop. Also, there already is a search of the channel list in vmbus_process_offer() so why is another lookup needed?
> -----Original Message----- > From: Stephen Hemminger <stephen@networkplumber.org> > Sent: Saturday, December 28, 2019 7:20 PM > To: Haiyang Zhang <haiyangz@microsoft.com> > Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; > KY Srinivasan <kys@microsoft.com>; Stephen Hemminger > <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets > <vkuznets@redhat.com>; davem@davemloft.net; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net-next, 1/3] Drivers: hv: vmbus: Add a dev_num variable > based on channel offer sequence > > On Sat, 28 Dec 2019 15:46:31 -0800 > Haiyang Zhang <haiyangz@microsoft.com> wrote: > > > + > > +next: > > + found = false; > > + > > + list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) { > > + if (i == channel->dev_num && > > + guid_equal(&channel->offermsg.offer.if_type, > > + &newchannel->offermsg.offer.if_type)) { > > + found = true; > > + break; > > + } > > + } > > + > > + if (found) { > > + i++; > > + goto next; > > + } > > + > > Overall, keeping track of dev_num is a good solution. > > I prefer not having a loop coded with goto's. Why not > a nested loop. Sure, I can use a nested loop. > Also, there already is a search of the channel > list in vmbus_process_offer() so why is another lookup needed? vmbus_process_offer() looks for the if_instance and if_type matches to determine if this is a subchannel vs primary channel. The loop terminates at different condition from hv_set_devnum(). So I didn't re-use the existing loop. And this kind of search happens only during channel offering, and doesn't impact performance much. Thanks, - Haiyang
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c index 8eb1675..b14c6a2 100644 --- a/drivers/hv/channel_mgmt.c +++ b/drivers/hv/channel_mgmt.c @@ -315,6 +315,8 @@ static struct vmbus_channel *alloc_channel(void) if (!channel) return NULL; + channel->dev_num = HV_DEV_NUM_INVALID; + spin_lock_init(&channel->lock); init_completion(&channel->rescind_event); @@ -541,6 +543,38 @@ static void vmbus_add_channel_work(struct work_struct *work) } /* + * Get the first available device number of its type, then + * record it in the channel structure. + */ +static void hv_set_devnum(struct vmbus_channel *newchannel) +{ + struct vmbus_channel *channel; + unsigned int i = 0; + bool found; + + BUG_ON(!mutex_is_locked(&vmbus_connection.channel_mutex)); + +next: + found = false; + + list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) { + if (i == channel->dev_num && + guid_equal(&channel->offermsg.offer.if_type, + &newchannel->offermsg.offer.if_type)) { + found = true; + break; + } + } + + if (found) { + i++; + goto next; + } + + newchannel->dev_num = i; +} + +/* * vmbus_process_offer - Process the offer by creating a channel/device * associated with this offer */ @@ -573,10 +607,12 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel) } } - if (fnew) + if (fnew) { + hv_set_devnum(newchannel); + list_add_tail(&newchannel->listentry, &vmbus_connection.chn_list); - else { + } else { /* * Check to see if this is a valid sub-channel. */ diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index 26f3aee..4f110c5 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -718,6 +718,8 @@ struct vmbus_device { bool perf_device; }; +#define HV_DEV_NUM_INVALID (-1) + struct vmbus_channel { struct list_head listentry; @@ -849,6 +851,10 @@ struct vmbus_channel { */ struct vmbus_channel *primary_channel; /* + * Used for device naming based on channel offer sequence. + */ + int dev_num; + /* * Support per-channel state for use by vmbus drivers. */ void *per_channel_state;
This number is set to the first available number, starting from zero, when a vmbus device’s primary channel is offered. It will be used for stable naming when Async probing is used. Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> --- drivers/hv/channel_mgmt.c | 40 ++++++++++++++++++++++++++++++++++++++-- include/linux/hyperv.h | 6 ++++++ 2 files changed, 44 insertions(+), 2 deletions(-)