| Message ID | 20170919193846.38060-3-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 19 Sep 2017, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote: > We should discourage developers from modifying modparams. > Introduce special macro for easier tracking of changes done > in modparams and enforce its use by defining existing modparams > members as const. Note that defining whole modparams struct > as const makes checkpatch unhappy. Checkpatch is the least of all reasons to not make the modparams struct const. We can get away with having some fields (such as device info within dev_priv) const, even if that's dubious. IIUC modifying const data is undefined behaviour at best, could cause subtle bugs through compiler optimizing reads of the data away because it assumes no modifications, and the data gets placed in rodata at worst. I kinda like the union trick in this patch, but IMO we need to double check what the standard says about it. Making the fellow developers check the standard is always a bad sign, even if it turns out to be fine after all. BR, Jani. > > v2: rebased > > Credits-to: Coccinelle > > @@ > identifier n; > expression e; > @@ > ( > - i915_modparams.n = e; > + i915_modparams_set(n, e); > ) > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Ville Syrjala <ville.syrjala@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 12 ++++++------ > drivers/gpu/drm/i915/i915_params.c | 4 ++-- > drivers/gpu/drm/i915/i915_params.h | 7 ++++++- > drivers/gpu/drm/i915/intel_fbc.c | 2 +- > drivers/gpu/drm/i915/intel_guc_log.c | 10 +++++----- > drivers/gpu/drm/i915/intel_gvt.c | 4 ++-- > drivers/gpu/drm/i915/intel_psr.c | 2 +- > drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- > drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++-- > drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++-------- > drivers/gpu/drm/i915/intel_uncore.c | 14 +++++++------- > 11 files changed, 43 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 7056bb2..99b47c5 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1032,9 +1032,9 @@ static void i915_driver_cleanup_mmio(struct drm_i915_private *dev_priv) > > static void intel_sanitize_options(struct drm_i915_private *dev_priv) > { > - i915_modparams.enable_execlists = > + i915_modparams_set(enable_execlists, > intel_sanitize_enable_execlists(dev_priv, > - i915_modparams.enable_execlists); > + i915_modparams.enable_execlists)); > > /* > * i915.enable_ppgtt is read-only, so do an early pass to validate the > @@ -1042,13 +1042,13 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv) > * do this now so that we can print out any log messages once rather > * than every time we check intel_enable_ppgtt(). > */ > - i915_modparams.enable_ppgtt = > + i915_modparams_set(enable_ppgtt, > intel_sanitize_enable_ppgtt(dev_priv, > - i915_modparams.enable_ppgtt); > + i915_modparams.enable_ppgtt)); > DRM_DEBUG_DRIVER("ppgtt mode: %i\n", i915_modparams.enable_ppgtt); > > - i915_modparams.semaphores = > - intel_sanitize_semaphores(dev_priv, i915_modparams.semaphores); > + i915_modparams_set(semaphores, > + intel_sanitize_semaphores(dev_priv, i915_modparams.semaphores)); > DRM_DEBUG_DRIVER("use GPU semaphores? %s\n", > yesno(i915_modparams.semaphores)); > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index ec65341..2a8fa9b 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -26,10 +26,10 @@ > #include "i915_drv.h" > > #define i915_param_named(name, T, perm, desc) \ > - module_param_named(name, i915_modparams.name, T, perm); \ > + module_param_named(name, i915_modparams.name##_writable, T, perm); \ > MODULE_PARM_DESC(name, desc) > #define i915_param_named_unsafe(name, T, perm, desc) \ > - module_param_named_unsafe(name, i915_modparams.name, T, perm); \ > + module_param_named_unsafe(name, i915_modparams.name##_writable, T, perm); \ > MODULE_PARM_DESC(name, desc) > > struct i915_params i915_modparams __read_mostly = { > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h > index a2cbb47..e7b2845 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -70,7 +70,7 @@ > func(bool, enable_dpcd_backlight); \ > func(bool, enable_gvt) > > -#define MEMBER(T, member) T member > +#define MEMBER(T, member) union { const T member; T member##_writable; } > struct i915_params { > I915_PARAMS_FOR_EACH(MEMBER); > }; > @@ -78,5 +78,10 @@ struct i915_params { > > extern struct i915_params i915_modparams __read_mostly; > > +#define i915_modparams_set(name, value) \ > +({ \ > + i915_modparams.name##_writable = value; \ > +}) > + > #endif > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 8e3a055..13fa354 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -1355,7 +1355,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv) > if (need_fbc_vtd_wa(dev_priv)) > mkwrite_device_info(dev_priv)->has_fbc = false; > > - i915_modparams.enable_fbc = intel_sanitize_fbc_option(dev_priv); > + i915_modparams_set(enable_fbc, intel_sanitize_fbc_option(dev_priv)); > DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n", > i915_modparams.enable_fbc); > > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c > index 6571d96..4876235 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.c > +++ b/drivers/gpu/drm/i915/intel_guc_log.c > @@ -480,7 +480,7 @@ static int guc_log_late_setup(struct intel_guc *guc) > guc_log_runtime_destroy(guc); > err: > /* logging will remain off */ > - i915_modparams.guc_log_level = -1; > + i915_modparams_set(guc_log_level, -1); > return ret; > } > > @@ -531,7 +531,7 @@ int intel_guc_log_create(struct intel_guc *guc) > GEM_BUG_ON(guc->log.vma); > > if (i915_modparams.guc_log_level > GUC_LOG_VERBOSITY_MAX) > - i915_modparams.guc_log_level = GUC_LOG_VERBOSITY_MAX; > + i915_modparams_set(guc_log_level, GUC_LOG_VERBOSITY_MAX); > > /* The first page is to save log buffer state. Allocate one > * extra page for others in case for overlap */ > @@ -577,7 +577,7 @@ int intel_guc_log_create(struct intel_guc *guc) > i915_vma_unpin_and_release(&guc->log.vma); > err: > /* logging will be off */ > - i915_modparams.guc_log_level = -1; > + i915_modparams_set(guc_log_level, -1); > return ret; > } > > @@ -611,7 +611,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) > } > > if (log_param.logging_enabled) { > - i915_modparams.guc_log_level = log_param.verbosity; > + i915_modparams_set(guc_log_level, log_param.verbosity); > > /* If log_level was set as -1 at boot time, then the relay channel file > * wouldn't have been created by now and interrupts also would not have > @@ -634,7 +634,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) > guc_flush_logs(guc); > > /* As logging is disabled, update log level to reflect that */ > - i915_modparams.guc_log_level = -1; > + i915_modparams_set(guc_log_level, -1); > } > > return ret; > diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c > index b4a7f31..2052e3e 100644 > --- a/drivers/gpu/drm/i915/intel_gvt.c > +++ b/drivers/gpu/drm/i915/intel_gvt.c > @@ -73,7 +73,7 @@ void intel_gvt_sanitize_options(struct drm_i915_private *dev_priv) > > return; > bail: > - i915_modparams.enable_gvt = 0; > + i915_modparams_set(enable_gvt, 0); > } > > /** > @@ -123,7 +123,7 @@ int intel_gvt_init(struct drm_i915_private *dev_priv) > return 0; > > bail: > - i915_modparams.enable_gvt = 0; > + i915_modparams_set(enable_gvt, 0); > return 0; > } > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 0a17d1f..2ec57d4d 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -944,7 +944,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv) > > /* Per platform default: all disabled. */ > if (i915_modparams.enable_psr == -1) > - i915_modparams.enable_psr = 0; > + i915_modparams_set(enable_psr, 0); > > /* Set link_standby x link_off defaults */ > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 05c08b0..a057d37 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1982,7 +1982,7 @@ static void intel_ring_init_semaphores(struct drm_i915_private *dev_priv, > i915_gem_object_put(obj); > err: > DRM_DEBUG_DRIVER("Failed to allocate space for semaphores, disabling\n"); > - i915_modparams.semaphores = 0; > + i915_modparams_set(semaphores, 0); > } > > static void intel_ring_init_irq(struct drm_i915_private *dev_priv, > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 7933d1b..05bb62c 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -2471,9 +2471,9 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv) > { > struct i915_power_domains *power_domains = &dev_priv->power_domains; > > - i915_modparams.disable_power_well = > + i915_modparams_set(disable_power_well, > sanitize_disable_power_well_option(dev_priv, > - i915_modparams.disable_power_well); > + i915_modparams.disable_power_well)); > dev_priv->csr.allowed_dc_mask = > get_allowed_dc_mask(dev_priv, i915_modparams.enable_dc); > > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 9018540..d91a441 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -67,14 +67,15 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) > i915_modparams.enable_guc_submission > 0) > DRM_INFO("Ignoring GuC options, no hardware\n"); > > - i915_modparams.enable_guc_loading = 0; > - i915_modparams.enable_guc_submission = 0; > + i915_modparams_set(enable_guc_loading, 0); > + i915_modparams_set(enable_guc_submission, 0); > return; > } > > /* A negative value means "use platform default" */ > if (i915_modparams.enable_guc_loading < 0) > - i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv); > + i915_modparams_set(enable_guc_loading, > + HAS_GUC_UCODE(dev_priv)); > > /* Verify firmware version */ > if (i915_modparams.enable_guc_loading) { > @@ -82,16 +83,17 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) > intel_huc_select_fw(&dev_priv->huc); > > if (intel_guc_select_fw(&dev_priv->guc)) > - i915_modparams.enable_guc_loading = 0; > + i915_modparams_set(enable_guc_loading, 0); > } > > /* Can't enable guc submission without guc loaded */ > if (!i915_modparams.enable_guc_loading) > - i915_modparams.enable_guc_submission = 0; > + i915_modparams_set(enable_guc_submission, 0); > > /* A negative value means "use platform default" */ > if (i915_modparams.enable_guc_submission < 0) > - i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv); > + i915_modparams_set(enable_guc_submission, > + HAS_GUC_SCHED(dev_priv)); > } > > static void gen8_guc_raise_irq(struct intel_guc *guc) > @@ -430,11 +432,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > ret = 0; > > if (i915_modparams.enable_guc_submission) { > - i915_modparams.enable_guc_submission = 0; > + i915_modparams_set(enable_guc_submission, 0); > DRM_NOTE("Falling back from GuC submission to execlist mode\n"); > } > > - i915_modparams.enable_guc_loading = 0; > + i915_modparams_set(enable_guc_loading, 0); > DRM_NOTE("GuC firmware loading disabled\n"); > > return ret; > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 79bbffa..20359a8 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -436,8 +436,8 @@ void intel_uncore_resume_early(struct drm_i915_private *dev_priv) > > void intel_uncore_sanitize(struct drm_i915_private *dev_priv) > { > - i915_modparams.enable_rc6 = > - sanitize_rc6_option(dev_priv, i915_modparams.enable_rc6); > + i915_modparams_set(enable_rc6, > + sanitize_rc6_option(dev_priv, i915_modparams.enable_rc6)); > > /* BIOS often leaves RC6 enabled, but disable it for hw init */ > intel_sanitize_gt_powersave(dev_priv); > @@ -511,7 +511,7 @@ void intel_uncore_forcewake_user_get(struct drm_i915_private *dev_priv) > i915_modparams.mmio_debug; > > dev_priv->uncore.unclaimed_mmio_check = 0; > - i915_modparams.mmio_debug = 0; > + i915_modparams_set(mmio_debug, 0); > } > spin_unlock_irq(&dev_priv->uncore.lock); > } > @@ -533,8 +533,8 @@ void intel_uncore_forcewake_user_put(struct drm_i915_private *dev_priv) > > dev_priv->uncore.unclaimed_mmio_check = > dev_priv->uncore.user_forcewake.saved_mmio_check; > - i915_modparams.mmio_debug = > - dev_priv->uncore.user_forcewake.saved_mmio_debug; > + i915_modparams_set(mmio_debug, > + dev_priv->uncore.user_forcewake.saved_mmio_debug); > > intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL); > } > @@ -843,7 +843,7 @@ __unclaimed_reg_debug(struct drm_i915_private *dev_priv, > read ? "read from" : "write to", > i915_mmio_reg_offset(reg))) > /* Only report the first N failures */ > - i915_modparams.mmio_debug--; > + i915_modparams_set(mmio_debug, i915_modparams.mmio_debug - 1); > } > > static inline void > @@ -1801,7 +1801,7 @@ intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv) > DRM_DEBUG("Unclaimed register detected, " > "enabling oneshot unclaimed register reporting. " > "Please use i915.mmio_debug=N for more information.\n"); > - i915_modparams.mmio_debug++; > + i915_modparams_set(mmio_debug, i915_modparams.mmio_debug + 1); > dev_priv->uncore.unclaimed_mmio_check--; > return true; > }
On Wed, 20 Sep 2017 10:34:43 +0200, Jani Nikula <jani.nikula@intel.com> wrote: > On Tue, 19 Sep 2017, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote: >> We should discourage developers from modifying modparams. >> Introduce special macro for easier tracking of changes done >> in modparams and enforce its use by defining existing modparams >> members as const. Note that defining whole modparams struct >> as const makes checkpatch unhappy. > > Checkpatch is the least of all reasons to not make the modparams struct > const. > > We can get away with having some fields (such as device info within > dev_priv) const, even if that's dubious. > > IIUC modifying const data is undefined behaviour at best, could cause > subtle bugs through compiler optimizing reads of the data away because > it assumes no modifications, and the data gets placed in rodata at > worst. Note that we're forcing modparams to be stored in dedicated section: #define __read_mostly __attribute__((__section__(".data..read_mostly"))) > > I kinda like the union trick in this patch, but IMO we need to double > check what the standard says about it. Making the fellow developers > check the standard is always a bad sign, even if it turns out to be fine > after all. ISO/IEC 9899: 6.5.2.3 If the member used to access the contents of a union object is not the same as the member last used to store a value in the object, the appropriate part of the object representation of the value is reinterpreted as an object representation in the new type as described in 6.2.6 (a process sometimes called "type punning"). Michal
On Tue, 2017-09-19 at 19:38 +0000, Michal Wajdeczko wrote: > We should discourage developers from modifying modparams. > Introduce special macro for easier tracking of changes done > in modparams and enforce its use by defining existing modparams > members as const. Note that defining whole modparams struct > as const makes checkpatch unhappy. > > v2: rebased > > Credits-to: Coccinelle > > @@ > identifier n; > expression e; > @@ > ( > - i915_modparams.n = e; > + i915_modparams_set(n, e); Not cool with such a brief name, it really needs to be something more standing out to make the developer think they've failed design if they're calling the function. 'i915_modparams_force_write' is my current favourite. And we need huge kerneldoc comment for the function about the concerns expressed by Jani, me and Ville. There must be no potential readers for the variables while they're being changed, compiler optimizations need to be watched for etc. Because really, if we change a module parameter variable while somebody is for example running a loop based on it, we're in deep problems. Might be worthwhile having a i915_modparams_lock to be taken when sanitization of options begins, and asserting that lock is held when _force_write() is being called. rw_semaphore sounds like the right choice here. Many can read but only one can write. Any opinions on that? Regards, Joonas
On Wed, Sep 20, 2017 at 11:54:03AM +0200, Michal Wajdeczko wrote: > On Wed, 20 Sep 2017 10:34:43 +0200, Jani Nikula <jani.nikula@intel.com> > wrote: > > > On Tue, 19 Sep 2017, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote: > >> We should discourage developers from modifying modparams. > >> Introduce special macro for easier tracking of changes done > >> in modparams and enforce its use by defining existing modparams > >> members as const. Note that defining whole modparams struct > >> as const makes checkpatch unhappy. > > > > Checkpatch is the least of all reasons to not make the modparams struct > > const. > > > > We can get away with having some fields (such as device info within > > dev_priv) const, even if that's dubious. > > > > IIUC modifying const data is undefined behaviour at best, could cause > > subtle bugs through compiler optimizing reads of the data away because > > it assumes no modifications, and the data gets placed in rodata at > > worst. > > Note that we're forcing modparams to be stored in dedicated section: > > #define __read_mostly __attribute__((__section__(".data..read_mostly"))) > > > > > I kinda like the union trick in this patch, but IMO we need to double > > check what the standard says about it. Making the fellow developers > > check the standard is always a bad sign, even if it turns out to be fine > > after all. > > ISO/IEC 9899: 6.5.2.3 > > If the member used to access the contents of a union object is not the same > as the member last used to store a value in the object, the appropriate > part > of the object representation of the value is reinterpreted as an object > representation in the new type as described in 6.2.6 (a process sometimes > called "type punning"). There's a bunch of text in 6.7.3 which may or may not make that part irrelevant. Very hard to say since there's no explicit example of this kind in the spec.
On Wed, 20 Sep 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > On Tue, 2017-09-19 at 19:38 +0000, Michal Wajdeczko wrote: >> We should discourage developers from modifying modparams. >> Introduce special macro for easier tracking of changes done >> in modparams and enforce its use by defining existing modparams >> members as const. Note that defining whole modparams struct >> as const makes checkpatch unhappy. >> >> v2: rebased >> >> Credits-to: Coccinelle >> >> @@ >> identifier n; >> expression e; >> @@ >> ( >> - i915_modparams.n = e; >> + i915_modparams_set(n, e); > > Not cool with such a brief name, it really needs to be something more > standing out to make the developer think they've failed design if > they're calling the function. > > 'i915_modparams_force_write' is my current favourite. > > And we need huge kerneldoc comment for the function about the concerns > expressed by Jani, me and Ville. There must be no potential readers for > the variables while they're being changed, compiler optimizations need > to be watched for etc. > > Because really, if we change a module parameter variable while somebody > is for example running a loop based on it, we're in deep problems. > > Might be worthwhile having a i915_modparams_lock to be taken when > sanitization of options begins, and asserting that lock is held when > _force_write() is being called. rw_semaphore sounds like the right > choice here. Many can read but only one can write. > > Any opinions on that? It can't protect against users changing the parameters via sysfs, and I think fixing that at the moment would have an air of overengineering. I'm thinking review and merge patch 1 to fix the i915 name collision, and forget about the rest for now. Too much controversy, no real rush or pressure to do anything right now beyond patch 1. Don't just do something, stand there. BR, Jani.
On Wed, 2017-09-20 at 11:34 +0300, Jani Nikula wrote: > On Tue, 19 Sep 2017, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote: > > We should discourage developers from modifying modparams. > > Introduce special macro for easier tracking of changes done > > in modparams and enforce its use by defining existing modparams > > members as const. Note that defining whole modparams struct > > as const makes checkpatch unhappy. > > Checkpatch is the least of all reasons to not make the modparams struct > const. > > We can get away with having some fields (such as device info within > dev_priv) const, even if that's dubious. > > IIUC modifying const data is undefined behaviour at best, could cause > subtle bugs through compiler optimizing reads of the data away because > it assumes no modifications, and the data gets placed in rodata at > worst. > > I kinda like the union trick in this patch, but IMO we need to double > check what the standard says about it. Making the fellow developers > check the standard is always a bad sign, even if it turns out to be fine > after all. Here's the snippet to describe the three discussed behaviors. Michal's code seems to do the right thing: https://gcc.godbolt.org/g/6MCNC3 We just need to make the write function stand out more and have a kerneldoc for it. Regards, Joonas
On Wed, 2017-09-20 at 15:06 +0300, Joonas Lahtinen wrote: > On Wed, 2017-09-20 at 11:34 +0300, Jani Nikula wrote: > > On Tue, 19 Sep 2017, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote: > > > We should discourage developers from modifying modparams. > > > Introduce special macro for easier tracking of changes done > > > in modparams and enforce its use by defining existing modparams > > > members as const. Note that defining whole modparams struct > > > as const makes checkpatch unhappy. > > > > Checkpatch is the least of all reasons to not make the modparams struct > > const. > > > > We can get away with having some fields (such as device info within > > dev_priv) const, even if that's dubious. > > > > IIUC modifying const data is undefined behaviour at best, could cause > > subtle bugs through compiler optimizing reads of the data away because > > it assumes no modifications, and the data gets placed in rodata at > > worst. > > > > I kinda like the union trick in this patch, but IMO we need to double > > check what the standard says about it. Making the fellow developers > > check the standard is always a bad sign, even if it turns out to be fine > > after all. > > Here's the snippet to describe the three discussed behaviors. Michal's > code seems to do the right thing: > > https://gcc.godbolt.org/g/6MCNC3 > > We just need to make the write function stand out more and have a > kerneldoc for it. > Umm, and when I don't typo a missing "&", it's more obvious (now with --Wall -Wextra -Werror): https://gcc.godbolt.org/g/HszLnw Regards, Joonas
On Wed, 2017-09-20 at 15:01 +0300, Jani Nikula wrote: > On Wed, 20 Sep 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > > On Tue, 2017-09-19 at 19:38 +0000, Michal Wajdeczko wrote: > > > We should discourage developers from modifying modparams. > > > Introduce special macro for easier tracking of changes done > > > in modparams and enforce its use by defining existing modparams > > > members as const. Note that defining whole modparams struct > > > as const makes checkpatch unhappy. > > > > > > v2: rebased > > > > > > Credits-to: Coccinelle > > > > > > @@ > > > identifier n; > > > expression e; > > > @@ > > > ( > > > - i915_modparams.n = e; > > > + i915_modparams_set(n, e); > > > > Not cool with such a brief name, it really needs to be something more > > standing out to make the developer think they've failed design if > > they're calling the function. > > > > 'i915_modparams_force_write' is my current favourite. > > > > And we need huge kerneldoc comment for the function about the concerns > > expressed by Jani, me and Ville. There must be no potential readers for > > the variables while they're being changed, compiler optimizations need > > to be watched for etc. > > > > Because really, if we change a module parameter variable while somebody > > is for example running a loop based on it, we're in deep problems. > > > > Might be worthwhile having a i915_modparams_lock to be taken when > > sanitization of options begins, and asserting that lock is held when > > _force_write() is being called. rw_semaphore sounds like the right > > choice here. Many can read but only one can write. > > > > Any opinions on that? > > It can't protect against users changing the parameters via sysfs, and I > think fixing that at the moment would have an air of overengineering. > > I'm thinking review and merge patch 1 to fix the i915 name collision, > and forget about the rest for now. Agreed on merging, disagreeing on forgetting next steps. > Too much controversy, no real rush or > pressure to do anything right now beyond patch 1. Don't just do > something, stand there. The controversy seemed to be around compiler optimizations, and that doesn't seem to be a worry. The other thing is how to name the function, and that's not too bad discussion. It naturally shouldn't block merging the first patch. Reviewing the places where the modparams get written/read may only lead to improvements as I see it. Any troublesome variables should get moved to device state instead of module state. For example while sanitizing enable_ppgtt and other user requested kernel parameters, we should copy the state to relevant dynamic structures where it'll have an effect, if we actually intend to support changing the parameters on the fly. Regards, Joonas
On Wed, 20 Sep 2017 15:07:50 +0200, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > On Wed, 2017-09-20 at 15:01 +0300, Jani Nikula wrote: >> On Wed, 20 Sep 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> wrote: >> > On Tue, 2017-09-19 at 19:38 +0000, Michal Wajdeczko wrote: >> > > We should discourage developers from modifying modparams. >> > > Introduce special macro for easier tracking of changes done >> > > in modparams and enforce its use by defining existing modparams >> > > members as const. Note that defining whole modparams struct >> > > as const makes checkpatch unhappy. >> > > >> > > v2: rebased >> > > >> > > Credits-to: Coccinelle >> > > >> > > @@ >> > > identifier n; >> > > expression e; >> > > @@ >> > > ( >> > > - i915_modparams.n = e; >> > > + i915_modparams_set(n, e); >> > >> > Not cool with such a brief name, it really needs to be something more >> > standing out to make the developer think they've failed design if >> > they're calling the function. >> > >> > 'i915_modparams_force_write' is my current favourite. >> > >> > And we need huge kerneldoc comment for the function about the concerns >> > expressed by Jani, me and Ville. There must be no potential readers >> for >> > the variables while they're being changed, compiler optimizations need >> > to be watched for etc. >> > >> > Because really, if we change a module parameter variable while >> somebody >> > is for example running a loop based on it, we're in deep problems. >> > >> > Might be worthwhile having a i915_modparams_lock to be taken when >> > sanitization of options begins, and asserting that lock is held when >> > _force_write() is being called. rw_semaphore sounds like the right >> > choice here. Many can read but only one can write. >> > >> > Any opinions on that? >> >> It can't protect against users changing the parameters via sysfs, and I >> think fixing that at the moment would have an air of overengineering. >> >> I'm thinking review and merge patch 1 to fix the i915 name collision, >> and forget about the rest for now. > > Agreed on merging, disagreeing on forgetting next steps. > >> Too much controversy, no real rush or >> pressure to do anything right now beyond patch 1. Don't just do >> something, stand there. > > The controversy seemed to be around compiler optimizations, and that > doesn't seem to be a worry. The other thing is how to name the > function, and that's not too bad discussion. It naturally shouldn't > block merging the first patch. If there is an agreement on merging first patch, can someone give it r-b and merge ? Note that this patch is prone to rebase conflicts. Michal > > Reviewing the places where the modparams get written/read may only lead > to improvements as I see it. Any troublesome variables should get moved > to device state instead of module state. For example while sanitizing > enable_ppgtt and other user requested kernel parameters, we should copy > the state to relevant dynamic structures where it'll have an effect, if > we actually intend to support changing the parameters on the fly. > > Regards, Joonas
On Wed, 20 Sep 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > On Wed, 2017-09-20 at 15:01 +0300, Jani Nikula wrote: >> On Wed, 20 Sep 2017, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: >> > On Tue, 2017-09-19 at 19:38 +0000, Michal Wajdeczko wrote: >> > > We should discourage developers from modifying modparams. >> > > Introduce special macro for easier tracking of changes done >> > > in modparams and enforce its use by defining existing modparams >> > > members as const. Note that defining whole modparams struct >> > > as const makes checkpatch unhappy. >> > > >> > > v2: rebased >> > > >> > > Credits-to: Coccinelle >> > > >> > > @@ >> > > identifier n; >> > > expression e; >> > > @@ >> > > ( >> > > - i915_modparams.n = e; >> > > + i915_modparams_set(n, e); >> > >> > Not cool with such a brief name, it really needs to be something more >> > standing out to make the developer think they've failed design if >> > they're calling the function. >> > >> > 'i915_modparams_force_write' is my current favourite. >> > >> > And we need huge kerneldoc comment for the function about the concerns >> > expressed by Jani, me and Ville. There must be no potential readers for >> > the variables while they're being changed, compiler optimizations need >> > to be watched for etc. >> > >> > Because really, if we change a module parameter variable while somebody >> > is for example running a loop based on it, we're in deep problems. >> > >> > Might be worthwhile having a i915_modparams_lock to be taken when >> > sanitization of options begins, and asserting that lock is held when >> > _force_write() is being called. rw_semaphore sounds like the right >> > choice here. Many can read but only one can write. >> > >> > Any opinions on that? >> >> It can't protect against users changing the parameters via sysfs, and I >> think fixing that at the moment would have an air of overengineering. >> >> I'm thinking review and merge patch 1 to fix the i915 name collision, >> and forget about the rest for now. > > Agreed on merging, disagreeing on forgetting next steps. > >> Too much controversy, no real rush or >> pressure to do anything right now beyond patch 1. Don't just do >> something, stand there. > > The controversy seemed to be around compiler optimizations, and that > doesn't seem to be a worry. The other thing is how to name the > function, and that's not too bad discussion. It naturally shouldn't > block merging the first patch. It's not just that. > Reviewing the places where the modparams get written/read may only lead > to improvements as I see it. Any troublesome variables should get moved > to device state instead of module state. For example while sanitizing > enable_ppgtt and other user requested kernel parameters, we should copy > the state to relevant dynamic structures where it'll have an effect, if > we actually intend to support changing the parameters on the fly. Please figure out the alternatives to changing module parameters first. The hurdles introduced in patch 3 aren't going to stop anyone from adding new ones. I'm not going to tell anyone to stop doing that until I have a better idea what to do *instead*. Overall I'm more concerned about the insane total number of module parameters that we have. We have them primarily because they are *convenient*, and they are pretty much the only way to force some behaviour on module probe. Anything you put in i915 debugfs or sysfs is going to be much less convenient for debugging. BR, Jani.
On Thu, 21 Sep 2017, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote: > If there is an agreement on merging first patch, can someone give it > r-b and merge ? Note that this patch is prone to rebase conflicts. Pushed the first patch, thanks. BR, Jani.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 7056bb2..99b47c5 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1032,9 +1032,9 @@ static void i915_driver_cleanup_mmio(struct drm_i915_private *dev_priv) static void intel_sanitize_options(struct drm_i915_private *dev_priv) { - i915_modparams.enable_execlists = + i915_modparams_set(enable_execlists, intel_sanitize_enable_execlists(dev_priv, - i915_modparams.enable_execlists); + i915_modparams.enable_execlists)); /* * i915.enable_ppgtt is read-only, so do an early pass to validate the @@ -1042,13 +1042,13 @@ static void intel_sanitize_options(struct drm_i915_private *dev_priv) * do this now so that we can print out any log messages once rather * than every time we check intel_enable_ppgtt(). */ - i915_modparams.enable_ppgtt = + i915_modparams_set(enable_ppgtt, intel_sanitize_enable_ppgtt(dev_priv, - i915_modparams.enable_ppgtt); + i915_modparams.enable_ppgtt)); DRM_DEBUG_DRIVER("ppgtt mode: %i\n", i915_modparams.enable_ppgtt); - i915_modparams.semaphores = - intel_sanitize_semaphores(dev_priv, i915_modparams.semaphores); + i915_modparams_set(semaphores, + intel_sanitize_semaphores(dev_priv, i915_modparams.semaphores)); DRM_DEBUG_DRIVER("use GPU semaphores? %s\n", yesno(i915_modparams.semaphores)); diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index ec65341..2a8fa9b 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -26,10 +26,10 @@ #include "i915_drv.h" #define i915_param_named(name, T, perm, desc) \ - module_param_named(name, i915_modparams.name, T, perm); \ + module_param_named(name, i915_modparams.name##_writable, T, perm); \ MODULE_PARM_DESC(name, desc) #define i915_param_named_unsafe(name, T, perm, desc) \ - module_param_named_unsafe(name, i915_modparams.name, T, perm); \ + module_param_named_unsafe(name, i915_modparams.name##_writable, T, perm); \ MODULE_PARM_DESC(name, desc) struct i915_params i915_modparams __read_mostly = { diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index a2cbb47..e7b2845 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -70,7 +70,7 @@ func(bool, enable_dpcd_backlight); \ func(bool, enable_gvt) -#define MEMBER(T, member) T member +#define MEMBER(T, member) union { const T member; T member##_writable; } struct i915_params { I915_PARAMS_FOR_EACH(MEMBER); }; @@ -78,5 +78,10 @@ struct i915_params { extern struct i915_params i915_modparams __read_mostly; +#define i915_modparams_set(name, value) \ +({ \ + i915_modparams.name##_writable = value; \ +}) + #endif diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 8e3a055..13fa354 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -1355,7 +1355,7 @@ void intel_fbc_init(struct drm_i915_private *dev_priv) if (need_fbc_vtd_wa(dev_priv)) mkwrite_device_info(dev_priv)->has_fbc = false; - i915_modparams.enable_fbc = intel_sanitize_fbc_option(dev_priv); + i915_modparams_set(enable_fbc, intel_sanitize_fbc_option(dev_priv)); DRM_DEBUG_KMS("Sanitized enable_fbc value: %d\n", i915_modparams.enable_fbc); diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 6571d96..4876235 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -480,7 +480,7 @@ static int guc_log_late_setup(struct intel_guc *guc) guc_log_runtime_destroy(guc); err: /* logging will remain off */ - i915_modparams.guc_log_level = -1; + i915_modparams_set(guc_log_level, -1); return ret; } @@ -531,7 +531,7 @@ int intel_guc_log_create(struct intel_guc *guc) GEM_BUG_ON(guc->log.vma); if (i915_modparams.guc_log_level > GUC_LOG_VERBOSITY_MAX) - i915_modparams.guc_log_level = GUC_LOG_VERBOSITY_MAX; + i915_modparams_set(guc_log_level, GUC_LOG_VERBOSITY_MAX); /* The first page is to save log buffer state. Allocate one * extra page for others in case for overlap */ @@ -577,7 +577,7 @@ int intel_guc_log_create(struct intel_guc *guc) i915_vma_unpin_and_release(&guc->log.vma); err: /* logging will be off */ - i915_modparams.guc_log_level = -1; + i915_modparams_set(guc_log_level, -1); return ret; } @@ -611,7 +611,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) } if (log_param.logging_enabled) { - i915_modparams.guc_log_level = log_param.verbosity; + i915_modparams_set(guc_log_level, log_param.verbosity); /* If log_level was set as -1 at boot time, then the relay channel file * wouldn't have been created by now and interrupts also would not have @@ -634,7 +634,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) guc_flush_logs(guc); /* As logging is disabled, update log level to reflect that */ - i915_modparams.guc_log_level = -1; + i915_modparams_set(guc_log_level, -1); } return ret; diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c index b4a7f31..2052e3e 100644 --- a/drivers/gpu/drm/i915/intel_gvt.c +++ b/drivers/gpu/drm/i915/intel_gvt.c @@ -73,7 +73,7 @@ void intel_gvt_sanitize_options(struct drm_i915_private *dev_priv) return; bail: - i915_modparams.enable_gvt = 0; + i915_modparams_set(enable_gvt, 0); } /** @@ -123,7 +123,7 @@ int intel_gvt_init(struct drm_i915_private *dev_priv) return 0; bail: - i915_modparams.enable_gvt = 0; + i915_modparams_set(enable_gvt, 0); return 0; } diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 0a17d1f..2ec57d4d 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -944,7 +944,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv) /* Per platform default: all disabled. */ if (i915_modparams.enable_psr == -1) - i915_modparams.enable_psr = 0; + i915_modparams_set(enable_psr, 0); /* Set link_standby x link_off defaults */ if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 05c08b0..a057d37 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1982,7 +1982,7 @@ static void intel_ring_init_semaphores(struct drm_i915_private *dev_priv, i915_gem_object_put(obj); err: DRM_DEBUG_DRIVER("Failed to allocate space for semaphores, disabling\n"); - i915_modparams.semaphores = 0; + i915_modparams_set(semaphores, 0); } static void intel_ring_init_irq(struct drm_i915_private *dev_priv, diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 7933d1b..05bb62c 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -2471,9 +2471,9 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv) { struct i915_power_domains *power_domains = &dev_priv->power_domains; - i915_modparams.disable_power_well = + i915_modparams_set(disable_power_well, sanitize_disable_power_well_option(dev_priv, - i915_modparams.disable_power_well); + i915_modparams.disable_power_well)); dev_priv->csr.allowed_dc_mask = get_allowed_dc_mask(dev_priv, i915_modparams.enable_dc); diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 9018540..d91a441 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -67,14 +67,15 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) i915_modparams.enable_guc_submission > 0) DRM_INFO("Ignoring GuC options, no hardware\n"); - i915_modparams.enable_guc_loading = 0; - i915_modparams.enable_guc_submission = 0; + i915_modparams_set(enable_guc_loading, 0); + i915_modparams_set(enable_guc_submission, 0); return; } /* A negative value means "use platform default" */ if (i915_modparams.enable_guc_loading < 0) - i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv); + i915_modparams_set(enable_guc_loading, + HAS_GUC_UCODE(dev_priv)); /* Verify firmware version */ if (i915_modparams.enable_guc_loading) { @@ -82,16 +83,17 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) intel_huc_select_fw(&dev_priv->huc); if (intel_guc_select_fw(&dev_priv->guc)) - i915_modparams.enable_guc_loading = 0; + i915_modparams_set(enable_guc_loading, 0); } /* Can't enable guc submission without guc loaded */ if (!i915_modparams.enable_guc_loading) - i915_modparams.enable_guc_submission = 0; + i915_modparams_set(enable_guc_submission, 0); /* A negative value means "use platform default" */ if (i915_modparams.enable_guc_submission < 0) - i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv); + i915_modparams_set(enable_guc_submission, + HAS_GUC_SCHED(dev_priv)); } static void gen8_guc_raise_irq(struct intel_guc *guc) @@ -430,11 +432,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) ret = 0; if (i915_modparams.enable_guc_submission) { - i915_modparams.enable_guc_submission = 0; + i915_modparams_set(enable_guc_submission, 0); DRM_NOTE("Falling back from GuC submission to execlist mode\n"); } - i915_modparams.enable_guc_loading = 0; + i915_modparams_set(enable_guc_loading, 0); DRM_NOTE("GuC firmware loading disabled\n"); return ret; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 79bbffa..20359a8 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -436,8 +436,8 @@ void intel_uncore_resume_early(struct drm_i915_private *dev_priv) void intel_uncore_sanitize(struct drm_i915_private *dev_priv) { - i915_modparams.enable_rc6 = - sanitize_rc6_option(dev_priv, i915_modparams.enable_rc6); + i915_modparams_set(enable_rc6, + sanitize_rc6_option(dev_priv, i915_modparams.enable_rc6)); /* BIOS often leaves RC6 enabled, but disable it for hw init */ intel_sanitize_gt_powersave(dev_priv); @@ -511,7 +511,7 @@ void intel_uncore_forcewake_user_get(struct drm_i915_private *dev_priv) i915_modparams.mmio_debug; dev_priv->uncore.unclaimed_mmio_check = 0; - i915_modparams.mmio_debug = 0; + i915_modparams_set(mmio_debug, 0); } spin_unlock_irq(&dev_priv->uncore.lock); } @@ -533,8 +533,8 @@ void intel_uncore_forcewake_user_put(struct drm_i915_private *dev_priv) dev_priv->uncore.unclaimed_mmio_check = dev_priv->uncore.user_forcewake.saved_mmio_check; - i915_modparams.mmio_debug = - dev_priv->uncore.user_forcewake.saved_mmio_debug; + i915_modparams_set(mmio_debug, + dev_priv->uncore.user_forcewake.saved_mmio_debug); intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL); } @@ -843,7 +843,7 @@ __unclaimed_reg_debug(struct drm_i915_private *dev_priv, read ? "read from" : "write to", i915_mmio_reg_offset(reg))) /* Only report the first N failures */ - i915_modparams.mmio_debug--; + i915_modparams_set(mmio_debug, i915_modparams.mmio_debug - 1); } static inline void @@ -1801,7 +1801,7 @@ intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv) DRM_DEBUG("Unclaimed register detected, " "enabling oneshot unclaimed register reporting. " "Please use i915.mmio_debug=N for more information.\n"); - i915_modparams.mmio_debug++; + i915_modparams_set(mmio_debug, i915_modparams.mmio_debug + 1); dev_priv->uncore.unclaimed_mmio_check--; return true; }
We should discourage developers from modifying modparams. Introduce special macro for easier tracking of changes done in modparams and enforce its use by defining existing modparams members as const. Note that defining whole modparams struct as const makes checkpatch unhappy. v2: rebased Credits-to: Coccinelle @@ identifier n; expression e; @@ ( - i915_modparams.n = e; + i915_modparams_set(n, e); ) Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Ville Syrjala <ville.syrjala@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 12 ++++++------ drivers/gpu/drm/i915/i915_params.c | 4 ++-- drivers/gpu/drm/i915/i915_params.h | 7 ++++++- drivers/gpu/drm/i915/intel_fbc.c | 2 +- drivers/gpu/drm/i915/intel_guc_log.c | 10 +++++----- drivers/gpu/drm/i915/intel_gvt.c | 4 ++-- drivers/gpu/drm/i915/intel_psr.c | 2 +- drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++-- drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++-------- drivers/gpu/drm/i915/intel_uncore.c | 14 +++++++------- 11 files changed, 43 insertions(+), 36 deletions(-)