| Message ID | 20200428205903.61704-21-urezki@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce kvfree_rcu(1 or 2 arguments) | expand |
On Tue, Apr 28, 2020 at 10:58:59PM +0200, Uladzislau Rezki (Sony) wrote: > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > Handle cases where the the object being kvfree_rcu()'d is not aligned by > 2-byte boundaries. > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > --- > kernel/rcu/tree.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 501cac02146d..649bad7ad0f0 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -2877,6 +2877,9 @@ struct kvfree_rcu_bulk_data { > #define KVFREE_BULK_MAX_ENTR \ > ((PAGE_SIZE - sizeof(struct kvfree_rcu_bulk_data)) / sizeof(void *)) > > +/* Encoding the offset of a fake rcu_head to indicate the head is a wrapper. */ > +#define RCU_HEADLESS_KFREE BIT(31) Did I miss the check for freeing something larger than 2GB? Or is this impossible, even on systems with many terabytes of physical memory? Even if it is currently impossible, what prevents it from suddenly becoming all too possible at some random point in the future? If you think that this will never happen, please keep in mind that the first time I heard "640K ought to be enough for anybody", it sounded eminently reasonable to me. Besides... Isn't the offset in question the offset of an rcu_head struct within the enclosing structure? If so, why not keep the current requirement that this be at least 16-bit aligned, especially given that some work is required to make that alignment less than pointer sized? Then you can continue using bit 0. This alignment requirement is included in the RCU requirements documentation and is enforced within the __call_rcu() function. So let's leave this at bit 0. Thanx, Paul > /** > * struct kfree_rcu_cpu_work - single batch of kfree_rcu() requests > * @rcu_work: Let queue_rcu_work() invoke workqueue handler after grace period > @@ -3078,9 +3081,9 @@ static void kfree_rcu_work(struct work_struct *work) > next = head->next; > > /* We tag the headless object, if so adjust offset. */ > - headless = (((unsigned long) head - offset) & BIT(0)); > + headless = !!(offset & RCU_HEADLESS_KFREE); > if (headless) > - offset -= 1; > + offset &= ~(RCU_HEADLESS_KFREE); > > ptr = (void *) head - offset; > > @@ -3356,7 +3359,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > * that has to be freed as well as dynamically > * attached wrapper/head. > */ > - func = (rcu_callback_t) (sizeof(unsigned long *) + 1); > + func = (rcu_callback_t)(sizeof(unsigned long *) | RCU_HEADLESS_KFREE); > } > > head->func = func; > -- > 2.20.1 >
On Fri, May 01, 2020 at 04:00:52PM -0700, Paul E. McKenney wrote: > On Tue, Apr 28, 2020 at 10:58:59PM +0200, Uladzislau Rezki (Sony) wrote: > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > > Handle cases where the the object being kvfree_rcu()'d is not aligned by > > 2-byte boundaries. > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > --- > > kernel/rcu/tree.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > index 501cac02146d..649bad7ad0f0 100644 > > --- a/kernel/rcu/tree.c > > +++ b/kernel/rcu/tree.c > > @@ -2877,6 +2877,9 @@ struct kvfree_rcu_bulk_data { > > #define KVFREE_BULK_MAX_ENTR \ > > ((PAGE_SIZE - sizeof(struct kvfree_rcu_bulk_data)) / sizeof(void *)) > > > > +/* Encoding the offset of a fake rcu_head to indicate the head is a wrapper. */ > > +#define RCU_HEADLESS_KFREE BIT(31) > > Did I miss the check for freeing something larger than 2GB? Or is this > impossible, even on systems with many terabytes of physical memory? > Even if it is currently impossible, what prevents it from suddenly > becoming all too possible at some random point in the future? If you > think that this will never happen, please keep in mind that the first > time I heard "640K ought to be enough for anybody", it sounded eminently > reasonable to me. > > Besides... > > Isn't the offset in question the offset of an rcu_head struct within > the enclosing structure? If so, why not keep the current requirement > that this be at least 16-bit aligned, especially given that some work > is required to make that alignment less than pointer sized? Then you > can continue using bit 0. > > This alignment requirement is included in the RCU requirements > documentation and is enforced within the __call_rcu() function. > > So let's leave this at bit 0. This patch is needed only if we are growing the fake rcu_head. Since you mentioned in a previous patch in this series that you don't want to do that, and just rely on availability of the array of pointers or synchronize_rcu(), we can drop this patch. If we are not dropping that earlier patch, let us discuss more. thanks, - Joel
On Sun, May 03, 2020 at 08:24:37PM -0400, Joel Fernandes wrote: > On Fri, May 01, 2020 at 04:00:52PM -0700, Paul E. McKenney wrote: > > On Tue, Apr 28, 2020 at 10:58:59PM +0200, Uladzislau Rezki (Sony) wrote: > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > > > > Handle cases where the the object being kvfree_rcu()'d is not aligned by > > > 2-byte boundaries. > > > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > --- > > > kernel/rcu/tree.c | 9 ++++++--- > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > index 501cac02146d..649bad7ad0f0 100644 > > > --- a/kernel/rcu/tree.c > > > +++ b/kernel/rcu/tree.c > > > @@ -2877,6 +2877,9 @@ struct kvfree_rcu_bulk_data { > > > #define KVFREE_BULK_MAX_ENTR \ > > > ((PAGE_SIZE - sizeof(struct kvfree_rcu_bulk_data)) / sizeof(void *)) > > > > > > +/* Encoding the offset of a fake rcu_head to indicate the head is a wrapper. */ > > > +#define RCU_HEADLESS_KFREE BIT(31) > > > > Did I miss the check for freeing something larger than 2GB? Or is this > > impossible, even on systems with many terabytes of physical memory? > > Even if it is currently impossible, what prevents it from suddenly > > becoming all too possible at some random point in the future? If you > > think that this will never happen, please keep in mind that the first > > time I heard "640K ought to be enough for anybody", it sounded eminently > > reasonable to me. > > > > Besides... > > > > Isn't the offset in question the offset of an rcu_head struct within > > the enclosing structure? If so, why not keep the current requirement > > that this be at least 16-bit aligned, especially given that some work > > is required to make that alignment less than pointer sized? Then you > > can continue using bit 0. > > > > This alignment requirement is included in the RCU requirements > > documentation and is enforced within the __call_rcu() function. > > > > So let's leave this at bit 0. > > This patch is needed only if we are growing the fake rcu_head. Since you > mentioned in a previous patch in this series that you don't want to do that, > and just rely on availability of the array of pointers or synchronize_rcu(), > we can drop this patch. If we are not dropping that earlier patch, let us > discuss more. Dropping it sounds very good to me! Thanx, Paul
On Sun, May 03, 2020 at 05:29:47PM -0700, Paul E. McKenney wrote: > On Sun, May 03, 2020 at 08:24:37PM -0400, Joel Fernandes wrote: > > On Fri, May 01, 2020 at 04:00:52PM -0700, Paul E. McKenney wrote: > > > On Tue, Apr 28, 2020 at 10:58:59PM +0200, Uladzislau Rezki (Sony) wrote: > > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > > > > > > Handle cases where the the object being kvfree_rcu()'d is not aligned by > > > > 2-byte boundaries. > > > > > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > --- > > > > kernel/rcu/tree.c | 9 ++++++--- > > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > index 501cac02146d..649bad7ad0f0 100644 > > > > --- a/kernel/rcu/tree.c > > > > +++ b/kernel/rcu/tree.c > > > > @@ -2877,6 +2877,9 @@ struct kvfree_rcu_bulk_data { > > > > #define KVFREE_BULK_MAX_ENTR \ > > > > ((PAGE_SIZE - sizeof(struct kvfree_rcu_bulk_data)) / sizeof(void *)) > > > > > > > > +/* Encoding the offset of a fake rcu_head to indicate the head is a wrapper. */ > > > > +#define RCU_HEADLESS_KFREE BIT(31) > > > > > > Did I miss the check for freeing something larger than 2GB? Or is this > > > impossible, even on systems with many terabytes of physical memory? > > > Even if it is currently impossible, what prevents it from suddenly > > > becoming all too possible at some random point in the future? If you > > > think that this will never happen, please keep in mind that the first > > > time I heard "640K ought to be enough for anybody", it sounded eminently > > > reasonable to me. > > > > > > Besides... > > > > > > Isn't the offset in question the offset of an rcu_head struct within > > > the enclosing structure? If so, why not keep the current requirement > > > that this be at least 16-bit aligned, especially given that some work > > > is required to make that alignment less than pointer sized? Then you > > > can continue using bit 0. > > > > > > This alignment requirement is included in the RCU requirements > > > documentation and is enforced within the __call_rcu() function. > > > > > > So let's leave this at bit 0. > > > > This patch is needed only if we are growing the fake rcu_head. Since you > > mentioned in a previous patch in this series that you don't want to do that, > > and just rely on availability of the array of pointers or synchronize_rcu(), > > we can drop this patch. If we are not dropping that earlier patch, let us > > discuss more. > > Dropping it sounds very good to me! Cool ;-) Thanks, - Joel
On Sun, May 03, 2020 at 08:31:06PM -0400, Joel Fernandes wrote: > On Sun, May 03, 2020 at 05:29:47PM -0700, Paul E. McKenney wrote: > > On Sun, May 03, 2020 at 08:24:37PM -0400, Joel Fernandes wrote: > > > On Fri, May 01, 2020 at 04:00:52PM -0700, Paul E. McKenney wrote: > > > > On Tue, Apr 28, 2020 at 10:58:59PM +0200, Uladzislau Rezki (Sony) wrote: > > > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > > > > > > > > Handle cases where the the object being kvfree_rcu()'d is not aligned by > > > > > 2-byte boundaries. > > > > > > > > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > --- > > > > > kernel/rcu/tree.c | 9 ++++++--- > > > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > > index 501cac02146d..649bad7ad0f0 100644 > > > > > --- a/kernel/rcu/tree.c > > > > > +++ b/kernel/rcu/tree.c > > > > > @@ -2877,6 +2877,9 @@ struct kvfree_rcu_bulk_data { > > > > > #define KVFREE_BULK_MAX_ENTR \ > > > > > ((PAGE_SIZE - sizeof(struct kvfree_rcu_bulk_data)) / sizeof(void *)) > > > > > > > > > > +/* Encoding the offset of a fake rcu_head to indicate the head is a wrapper. */ > > > > > +#define RCU_HEADLESS_KFREE BIT(31) > > > > > > > > Did I miss the check for freeing something larger than 2GB? Or is this > > > > impossible, even on systems with many terabytes of physical memory? > > > > Even if it is currently impossible, what prevents it from suddenly > > > > becoming all too possible at some random point in the future? If you > > > > think that this will never happen, please keep in mind that the first > > > > time I heard "640K ought to be enough for anybody", it sounded eminently > > > > reasonable to me. > > > > > > > > Besides... > > > > > > > > Isn't the offset in question the offset of an rcu_head struct within > > > > the enclosing structure? If so, why not keep the current requirement > > > > that this be at least 16-bit aligned, especially given that some work > > > > is required to make that alignment less than pointer sized? Then you > > > > can continue using bit 0. > > > > > > > > This alignment requirement is included in the RCU requirements > > > > documentation and is enforced within the __call_rcu() function. > > > > > > > > So let's leave this at bit 0. > > > > > > This patch is needed only if we are growing the fake rcu_head. Since you > > > mentioned in a previous patch in this series that you don't want to do that, > > > and just rely on availability of the array of pointers or synchronize_rcu(), > > > we can drop this patch. If we are not dropping that earlier patch, let us > > > discuss more. > > > > Dropping it sounds very good to me! > > Cool ;-) Thanks, > OK. Then we drop this patch and all dynamic rcu_head attaching logic what will make the code size smaller. Thanks! -- Vlad Rezki
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 501cac02146d..649bad7ad0f0 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -2877,6 +2877,9 @@ struct kvfree_rcu_bulk_data { #define KVFREE_BULK_MAX_ENTR \ ((PAGE_SIZE - sizeof(struct kvfree_rcu_bulk_data)) / sizeof(void *)) +/* Encoding the offset of a fake rcu_head to indicate the head is a wrapper. */ +#define RCU_HEADLESS_KFREE BIT(31) + /** * struct kfree_rcu_cpu_work - single batch of kfree_rcu() requests * @rcu_work: Let queue_rcu_work() invoke workqueue handler after grace period @@ -3078,9 +3081,9 @@ static void kfree_rcu_work(struct work_struct *work) next = head->next; /* We tag the headless object, if so adjust offset. */ - headless = (((unsigned long) head - offset) & BIT(0)); + headless = !!(offset & RCU_HEADLESS_KFREE); if (headless) - offset -= 1; + offset &= ~(RCU_HEADLESS_KFREE); ptr = (void *) head - offset; @@ -3356,7 +3359,7 @@ void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) * that has to be freed as well as dynamically * attached wrapper/head. */ - func = (rcu_callback_t) (sizeof(unsigned long *) + 1); + func = (rcu_callback_t)(sizeof(unsigned long *) | RCU_HEADLESS_KFREE); } head->func = func;