| Message ID | 20220413204742.5539-7-parri.andrea@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hv_sock: Hardening changes | expand |
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 13, 2022 1:48 PM > > With no users of hv_pkt_iter_next_raw() and no "external" users of > hv_pkt_iter_first_raw(), the iterator functions can be refactored > and simplified to remove some indirection/code. > > Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> > --- > drivers/hv/ring_buffer.c | 11 +++++------ > include/linux/hyperv.h | 35 ++++------------------------------- > 2 files changed, 9 insertions(+), 37 deletions(-) > > diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c > index 3d215d9dec433..c9357dae2a2c8 100644 > --- a/drivers/hv/ring_buffer.c > +++ b/drivers/hv/ring_buffer.c > @@ -421,7 +421,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel, > memcpy(buffer, (const char *)desc + offset, packetlen); > > /* Advance ring index to next packet descriptor */ > - __hv_pkt_iter_next(channel, desc, true); > + __hv_pkt_iter_next(channel, desc); > > /* Notify host of update */ > hv_pkt_iter_close(channel); > @@ -459,7 +459,8 @@ static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info > *rbi) > /* > * Get first vmbus packet without copying it out of the ring buffer > */ > -struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct vmbus_channel *channel) > +static struct vmpacket_descriptor * > +hv_pkt_iter_first_raw(struct vmbus_channel *channel) > { > struct hv_ring_buffer_info *rbi = &channel->inbound; > > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct > vmbus_channel *channel) > > return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi- > >priv_read_index); > } > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw); Does hv_pkt_iter_first_raw() need to be retained at all as a separate function? I think after these changes, the only caller is hv_pkt_iter_first(), in which case the code could just go inline in hv_pkt_iter_first(). Doing that combining would also allow the elimination of the duplicate call to hv_pkt_iter_avail(). Michael > > /* > * Get first vmbus packet from ring buffer after read_index > @@ -534,8 +534,7 @@ EXPORT_SYMBOL_GPL(hv_pkt_iter_first); > */ > struct vmpacket_descriptor * > __hv_pkt_iter_next(struct vmbus_channel *channel, > - const struct vmpacket_descriptor *desc, > - bool copy) > + const struct vmpacket_descriptor *desc) > { > struct hv_ring_buffer_info *rbi = &channel->inbound; > u32 packetlen = desc->len8 << 3; > @@ -548,7 +547,7 @@ __hv_pkt_iter_next(struct vmbus_channel *channel, > rbi->priv_read_index -= dsize; > > /* more data? */ > - return copy ? hv_pkt_iter_first(channel) : hv_pkt_iter_first_raw(channel); > + return hv_pkt_iter_first(channel); > } > EXPORT_SYMBOL_GPL(__hv_pkt_iter_next); > > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > index 1112c5cf894e6..370adc9971d3e 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -1673,55 +1673,28 @@ static inline u32 hv_pkt_len(const struct > vmpacket_descriptor *desc) > return desc->len8 << 3; > } > > -struct vmpacket_descriptor * > -hv_pkt_iter_first_raw(struct vmbus_channel *channel); > - > struct vmpacket_descriptor * > hv_pkt_iter_first(struct vmbus_channel *channel); > > struct vmpacket_descriptor * > __hv_pkt_iter_next(struct vmbus_channel *channel, > - const struct vmpacket_descriptor *pkt, > - bool copy); > + const struct vmpacket_descriptor *pkt); > > void hv_pkt_iter_close(struct vmbus_channel *channel); > > static inline struct vmpacket_descriptor * > -hv_pkt_iter_next_pkt(struct vmbus_channel *channel, > - const struct vmpacket_descriptor *pkt, > - bool copy) > +hv_pkt_iter_next(struct vmbus_channel *channel, > + const struct vmpacket_descriptor *pkt) > { > struct vmpacket_descriptor *nxt; > > - nxt = __hv_pkt_iter_next(channel, pkt, copy); > + nxt = __hv_pkt_iter_next(channel, pkt); > if (!nxt) > hv_pkt_iter_close(channel); > > return nxt; > } > > -/* > - * Get next packet descriptor without copying it out of the ring buffer > - * If at end of list, return NULL and update host. > - */ > -static inline struct vmpacket_descriptor * > -hv_pkt_iter_next_raw(struct vmbus_channel *channel, > - const struct vmpacket_descriptor *pkt) > -{ > - return hv_pkt_iter_next_pkt(channel, pkt, false); > -} > - > -/* > - * Get next packet descriptor from iterator > - * If at end of list, return NULL and update host. > - */ > -static inline struct vmpacket_descriptor * > -hv_pkt_iter_next(struct vmbus_channel *channel, > - const struct vmpacket_descriptor *pkt) > -{ > - return hv_pkt_iter_next_pkt(channel, pkt, true); > -} > - > #define foreach_vmbus_pkt(pkt, channel) \ > for (pkt = hv_pkt_iter_first(channel); pkt; \ > pkt = hv_pkt_iter_next(channel, pkt)) > -- > 2.25.1
> > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct > > vmbus_channel *channel) > > > > return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi- > > >priv_read_index); > > } > > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw); > > Does hv_pkt_iter_first_raw() need to be retained at all as a > separate function? I think after these changes, the only caller > is hv_pkt_iter_first(), in which case the code could just go > inline in hv_pkt_iter_first(). Doing that combining would > also allow the elimination of the duplicate call to > hv_pkt_iter_avail(). Good point. Will do. Thanks, Andrea
On Fri, Apr 15, 2022 at 09:00:31AM +0200, Andrea Parri wrote: > > > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct > > > vmbus_channel *channel) > > > > > > return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi- > > > >priv_read_index); > > > } > > > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw); > > > > Does hv_pkt_iter_first_raw() need to be retained at all as a > > separate function? I think after these changes, the only caller > > is hv_pkt_iter_first(), in which case the code could just go > > inline in hv_pkt_iter_first(). Doing that combining would > > also allow the elimination of the duplicate call to > > hv_pkt_iter_avail(). Back to this, can you clarify what you mean by "the elimination of..."? After moving the function "inline", hv_pkt_iter_avail() would be called in to check for a non-NULL descriptor (in the inline function) and later in the computation of bytes_avail. Thanks, Andrea > > Good point. Will do. > > Thanks, > Andrea
From: Andrea Parri <parri.andrea@gmail.com> Sent: Friday, April 15, 2022 9:28 AM > > On Fri, Apr 15, 2022 at 09:00:31AM +0200, Andrea Parri wrote: > > > > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct > > > > vmbus_channel *channel) > > > > > > > > return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi- > > > > >priv_read_index); > > > > } > > > > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw); > > > > > > Does hv_pkt_iter_first_raw() need to be retained at all as a > > > separate function? I think after these changes, the only caller > > > is hv_pkt_iter_first(), in which case the code could just go > > > inline in hv_pkt_iter_first(). Doing that combining would > > > also allow the elimination of the duplicate call to > > > hv_pkt_iter_avail(). > > Back to this, can you clarify what you mean by "the elimination of..."? > After moving the function "inline", hv_pkt_iter_avail() would be called > in to check for a non-NULL descriptor (in the inline function) and later > in the computation of bytes_avail. I was thinking something like this: bytes_avail = hv_pkt_iter_avail(rbi); if (bytes_avail < sizeof(struct vmpacket_descriptor)) return NULL; bytes_avail = min(rbi->pkt_buffer_size, bytes_avail); desc = (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi->priv_read_index); And for that matter, hv_pkt_iter_avail() is now only called in one place. It's a judgment call whether to keep it as a separate helper function vs. inlining it in hv_pkt_iter_first() as well. I'm OK either way. Michael
On Fri, Apr 15, 2022 at 04:44:50PM +0000, Michael Kelley (LINUX) wrote: > From: Andrea Parri <parri.andrea@gmail.com> Sent: Friday, April 15, 2022 9:28 AM > > > > On Fri, Apr 15, 2022 at 09:00:31AM +0200, Andrea Parri wrote: > > > > > @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct > > > > > vmbus_channel *channel) > > > > > > > > > > return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi- > > > > > >priv_read_index); > > > > > } > > > > > -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw); > > > > > > > > Does hv_pkt_iter_first_raw() need to be retained at all as a > > > > separate function? I think after these changes, the only caller > > > > is hv_pkt_iter_first(), in which case the code could just go > > > > inline in hv_pkt_iter_first(). Doing that combining would > > > > also allow the elimination of the duplicate call to > > > > hv_pkt_iter_avail(). > > > > Back to this, can you clarify what you mean by "the elimination of..."? > > After moving the function "inline", hv_pkt_iter_avail() would be called > > in to check for a non-NULL descriptor (in the inline function) and later > > in the computation of bytes_avail. > > I was thinking something like this: > > bytes_avail = hv_pkt_iter_avail(rbi); > if (bytes_avail < sizeof(struct vmpacket_descriptor)) > return NULL; > bytes_avail = min(rbi->pkt_buffer_size, bytes_avail); > > desc = (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi->priv_read_index); Thanks for the clarification, I've applied it. Andrea > And for that matter, hv_pkt_iter_avail() is now only called in one place. > It's a judgment call whether to keep it as a separate helper function vs. > inlining it in hv_pkt_iter_first() as well. I'm OK either way. > > > Michael > >
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c index 3d215d9dec433..c9357dae2a2c8 100644 --- a/drivers/hv/ring_buffer.c +++ b/drivers/hv/ring_buffer.c @@ -421,7 +421,7 @@ int hv_ringbuffer_read(struct vmbus_channel *channel, memcpy(buffer, (const char *)desc + offset, packetlen); /* Advance ring index to next packet descriptor */ - __hv_pkt_iter_next(channel, desc, true); + __hv_pkt_iter_next(channel, desc); /* Notify host of update */ hv_pkt_iter_close(channel); @@ -459,7 +459,8 @@ static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info *rbi) /* * Get first vmbus packet without copying it out of the ring buffer */ -struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct vmbus_channel *channel) +static struct vmpacket_descriptor * +hv_pkt_iter_first_raw(struct vmbus_channel *channel) { struct hv_ring_buffer_info *rbi = &channel->inbound; @@ -470,7 +471,6 @@ struct vmpacket_descriptor *hv_pkt_iter_first_raw(struct vmbus_channel *channel) return (struct vmpacket_descriptor *)(hv_get_ring_buffer(rbi) + rbi->priv_read_index); } -EXPORT_SYMBOL_GPL(hv_pkt_iter_first_raw); /* * Get first vmbus packet from ring buffer after read_index @@ -534,8 +534,7 @@ EXPORT_SYMBOL_GPL(hv_pkt_iter_first); */ struct vmpacket_descriptor * __hv_pkt_iter_next(struct vmbus_channel *channel, - const struct vmpacket_descriptor *desc, - bool copy) + const struct vmpacket_descriptor *desc) { struct hv_ring_buffer_info *rbi = &channel->inbound; u32 packetlen = desc->len8 << 3; @@ -548,7 +547,7 @@ __hv_pkt_iter_next(struct vmbus_channel *channel, rbi->priv_read_index -= dsize; /* more data? */ - return copy ? hv_pkt_iter_first(channel) : hv_pkt_iter_first_raw(channel); + return hv_pkt_iter_first(channel); } EXPORT_SYMBOL_GPL(__hv_pkt_iter_next); diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index 1112c5cf894e6..370adc9971d3e 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -1673,55 +1673,28 @@ static inline u32 hv_pkt_len(const struct vmpacket_descriptor *desc) return desc->len8 << 3; } -struct vmpacket_descriptor * -hv_pkt_iter_first_raw(struct vmbus_channel *channel); - struct vmpacket_descriptor * hv_pkt_iter_first(struct vmbus_channel *channel); struct vmpacket_descriptor * __hv_pkt_iter_next(struct vmbus_channel *channel, - const struct vmpacket_descriptor *pkt, - bool copy); + const struct vmpacket_descriptor *pkt); void hv_pkt_iter_close(struct vmbus_channel *channel); static inline struct vmpacket_descriptor * -hv_pkt_iter_next_pkt(struct vmbus_channel *channel, - const struct vmpacket_descriptor *pkt, - bool copy) +hv_pkt_iter_next(struct vmbus_channel *channel, + const struct vmpacket_descriptor *pkt) { struct vmpacket_descriptor *nxt; - nxt = __hv_pkt_iter_next(channel, pkt, copy); + nxt = __hv_pkt_iter_next(channel, pkt); if (!nxt) hv_pkt_iter_close(channel); return nxt; } -/* - * Get next packet descriptor without copying it out of the ring buffer - * If at end of list, return NULL and update host. - */ -static inline struct vmpacket_descriptor * -hv_pkt_iter_next_raw(struct vmbus_channel *channel, - const struct vmpacket_descriptor *pkt) -{ - return hv_pkt_iter_next_pkt(channel, pkt, false); -} - -/* - * Get next packet descriptor from iterator - * If at end of list, return NULL and update host. - */ -static inline struct vmpacket_descriptor * -hv_pkt_iter_next(struct vmbus_channel *channel, - const struct vmpacket_descriptor *pkt) -{ - return hv_pkt_iter_next_pkt(channel, pkt, true); -} - #define foreach_vmbus_pkt(pkt, channel) \ for (pkt = hv_pkt_iter_first(channel); pkt; \ pkt = hv_pkt_iter_next(channel, pkt))
With no users of hv_pkt_iter_next_raw() and no "external" users of hv_pkt_iter_first_raw(), the iterator functions can be refactored and simplified to remove some indirection/code. Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> --- drivers/hv/ring_buffer.c | 11 +++++------ include/linux/hyperv.h | 35 ++++------------------------------- 2 files changed, 9 insertions(+), 37 deletions(-)